crates/stdlib/src/_asyncio.rs (1)

501-515: Extract the shared cancelled-error helper.

These two implementations are now byte-for-byte identical. Pulling the common logic into one helper would make future asyncio parity fixes much less likely to drift between PyFuture and PyTask.

♻️ Possible extraction

+fn take_or_create_cancelled_error(
+    cancelled_exc: &PyRwLock<Option<PyObjectRef>>,
+    cancel_msg: &PyRwLock<Option<PyObjectRef>>,
+    vm: &VirtualMachine,
+) -> PyBaseExceptionRef {
+    if let Some(exc) = cancelled_exc.write().take()
+        && let Ok(exc) = exc.downcast::<PyBaseException>()
+    {
+        return exc;
+    }
+
+    let args = cancel_msg.read().clone().into_iter().collect::<Vec<_>>();
+    match get_cancelled_error_type(vm) {
+        Ok(cancelled_error) => vm.new_exception(cancelled_error, args),
+        Err(_) => vm.new_runtime_error("cancelled"),
+    }
+}

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

Also applies to: 1310-1324

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/_asyncio.rs` around lines 501 - 515, Both
PyFuture::make_cancelled_error_impl and PyTask::make_cancelled_error_impl
contain identical logic; extract a single helper (e.g.,
make_cancelled_error_helper) that takes the shared inputs (the stored
cancelled-exception cell/reference and the cancelled-message cell/reference plus
&VirtualMachine) and returns PyBaseExceptionRef using the same logic: take and
downcast fut_cancelled_exc, fallback to reading fut_cancel_msg and building
args, then call get_cancelled_error_type(vm) to construct the exception or
vm.new_runtime_error("cancelled"). Replace both existing
make_cancelled_error_impl bodies to call this new helper (and update the other
duplicated block around 1310-1324 similarly), keeping the helper visible to both
PyFuture and PyTask implementations.

crates/vm/src/gc_state.rs (2)

425-440: Consider extracting duplicate reset-and-return logic into a helper.

The same pattern appears four times: calculate reset_end, loop to reset counts, compute duration, update stats, and return CollectResult. This violates DRY.

♻️ Suggested refactor to reduce duplication

// Add this helper method to GcState
fn finalize_collection(
    &self,
    generation: usize,
    collected: usize,
    uncollectable: usize,
    candidates: usize,
    start_time: &impl TimingStart,
) -> CollectResult {
    let reset_end = if generation >= 2 { 2 } else { generation + 1 };
    for i in 0..reset_end {
        self.generations[i].count.store(0, Ordering::SeqCst);
    }
    let duration = elapsed_secs(start_time);
    self.generations[generation].update_stats(collected, uncollectable, candidates, duration);
    CollectResult {
        collected,
        uncollectable,
        candidates,
        duration,
    }
}

Then replace each occurrence with a call to this helper.

Also applies to: 536-551, 556-572, 705-720

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/gc_state.rs` around lines 425 - 440, Extract the repeated
reset-and-return pattern into a GcState helper (e.g., fn
finalize_collection(&self, generation: usize, collected: usize, uncollectable:
usize, candidates: usize, start_time: &impl TimingStart) -> CollectResult) that
computes reset_end (if generation >= 2 { 2 } else { generation + 1 }), loops
over self.generations[0..reset_end] to store(0, Ordering::SeqCst), calls
elapsed_secs(start_time), calls
self.generations[generation].update_stats(collected, uncollectable, candidates,
duration), and returns the CollectResult; then replace the four duplicate blocks
(the ones that currently perform the reset loop, call elapsed_secs,
update_stats, and return CollectResult) with calls to finalize_collection to
remove the duplication.

---

713-713: uncollectable is always 0 in stats updates.

Looking at all update_stats calls and CollectResult constructions, uncollectable is hardcoded to 0. If uncollectable object detection isn't implemented yet, consider adding a TODO comment to clarify this is intentional.

-        self.generations[generation].update_stats(collected, 0, candidates, duration);
+        // TODO: Uncollectable detection (objects with __del__ in cycles) not yet implemented
+        self.generations[generation].update_stats(collected, 0, candidates, duration);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/gc_state.rs` at line 713, The call to
self.generations[generation].update_stats(collected, 0, candidates, duration)
always passes 0 for the uncollectable count; either compute and pass the real
uncollectable count from the corresponding CollectResult (or other collector
return value) and propagate that value into update_stats, or if uncollectable
detection is not implemented yet, replace the hardcoded 0 with a named variable
(e.g., uncollectable) and add a TODO comment near update_stats / CollectResult
construction clarifying that uncollectable counting is not yet implemented and
should be filled in when implemented; refer to update_stats and CollectResult to
locate where to change the call and where to surface the TODO.

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

7652-7660: Restore the previous in_inlined_comp state instead of forcing false.

Line 7659 hard-resets the flag. That works with today's no-nested-scope policy, but it will drop an outer true if this path ever becomes re-entrant. Saving and restoring the old value keeps the state transition local.

♻️ Suggested tweak

-            self.current_code_info().in_inlined_comp = true;
+            let was_in_inlined_comp = self.current_code_info().in_inlined_comp;
+            self.current_code_info().in_inlined_comp = true;
             let result = self.compile_inlined_comprehension(
                 init_collection,
                 generators,
                 compile_element,
                 has_an_async_gen,
             );
-            self.current_code_info().in_inlined_comp = false;
+            self.current_code_info().in_inlined_comp = was_in_inlined_comp;
             return result;

🤖 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 7652 - 7660, The current code
unconditionally sets self.current_code_info().in_inlined_comp = false after
calling compile_inlined_comprehension which can clobber an outer true; instead
capture the previous value (e.g., let old =
self.current_code_info().in_inlined_comp), set in_inlined_comp = true before
calling compile_inlined_comprehension, then restore it to old afterwards so the
state change is local to this call.

---

7849-7865: Add a regression test that rewinds next_sub_table.

This helper now owns the cursor advance for inlined comprehensions. A case that compiles the same AST twice—an inlined comprehension inside finally is the easiest one here—would lock down the reset logic in compile_try_statement and protect this fix from future cursor regressions.

🤖 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 7849 - 7865, The regression is
that advancing next_sub_table when inlining a comprehension can leave the symbol
table cursor moved for subsequent compilations of the same AST; add a regression
test that compiles the same AST twice (best case: an inlined comprehension
inside a finally block) to ensure compile_try_statement properly rewinds
next_sub_table on exit. In the test, exercise compile_try_statement (via a small
snippet that triggers an inlined comprehension in a finally) and assert that
after the first compilation a second compilation succeeds without panicking or
mis-indexing symbol_table_stack.next_sub_table; this ensures the fix that
mutates next_sub_table in the comprehension-splicing code is protected against
cursor regressions.

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7652-7660: The current code unconditionally sets
self.current_code_info().in_inlined_comp = false after calling
compile_inlined_comprehension which can clobber an outer true; instead capture
the previous value (e.g., let old = self.current_code_info().in_inlined_comp),
set in_inlined_comp = true before calling compile_inlined_comprehension, then
restore it to old afterwards so the state change is local to this call.
- Around line 7849-7865: The regression is that advancing next_sub_table when
inlining a comprehension can leave the symbol table cursor moved for subsequent
compilations of the same AST; add a regression test that compiles the same AST
twice (best case: an inlined comprehension inside a finally block) to ensure
compile_try_statement properly rewinds next_sub_table on exit. In the test,
exercise compile_try_statement (via a small snippet that triggers an inlined
comprehension in a finally) and assert that after the first compilation a second
compilation succeeds without panicking or mis-indexing
symbol_table_stack.next_sub_table; this ensures the fix that mutates
next_sub_table in the comprehension-splicing code is protected against cursor
regressions.

In `@crates/stdlib/src/_asyncio.rs`:
- Around line 501-515: Both PyFuture::make_cancelled_error_impl and
PyTask::make_cancelled_error_impl contain identical logic; extract a single
helper (e.g., make_cancelled_error_helper) that takes the shared inputs (the
stored cancelled-exception cell/reference and the cancelled-message
cell/reference plus &VirtualMachine) and returns PyBaseExceptionRef using the
same logic: take and downcast fut_cancelled_exc, fallback to reading
fut_cancel_msg and building args, then call get_cancelled_error_type(vm) to
construct the exception or vm.new_runtime_error("cancelled"). Replace both
existing make_cancelled_error_impl bodies to call this new helper (and update
the other duplicated block around 1310-1324 similarly), keeping the helper
visible to both PyFuture and PyTask implementations.

In `@crates/vm/src/gc_state.rs`:
- Around line 425-440: Extract the repeated reset-and-return pattern into a
GcState helper (e.g., fn finalize_collection(&self, generation: usize,
collected: usize, uncollectable: usize, candidates: usize, start_time: &impl
TimingStart) -> CollectResult) that computes reset_end (if generation >= 2 { 2 }
else { generation + 1 }), loops over self.generations[0..reset_end] to store(0,
Ordering::SeqCst), calls elapsed_secs(start_time), calls
self.generations[generation].update_stats(collected, uncollectable, candidates,
duration), and returns the CollectResult; then replace the four duplicate blocks
(the ones that currently perform the reset loop, call elapsed_secs,
update_stats, and return CollectResult) with calls to finalize_collection to
remove the duplication.
- Line 713: The call to self.generations[generation].update_stats(collected, 0,
candidates, duration) always passes 0 for the uncollectable count; either
compute and pass the real uncollectable count from the corresponding
CollectResult (or other collector return value) and propagate that value into
update_stats, or if uncollectable detection is not implemented yet, replace the
hardcoded 0 with a named variable (e.g., uncollectable) and add a TODO comment
near update_stats / CollectResult construction clarifying that uncollectable
counting is not yet implemented and should be filled in when implemented; refer
to update_stats and CollectResult to locate where to change the call and where
to surface the TODO.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 43f83f0b-5f5a-467e-996a-810da133e093

Reviewing files that changed from the base of the PR and between f26752c and e58f796.

• Lib/test/_test_gc_fast_cycles.py is excluded by !Lib/**
• Lib/test/test_gc.py is excluded by !Lib/**
• Lib/test/test_symtable.py is excluded by !Lib/**

• crates/codegen/src/compile.rs
• crates/codegen/src/symboltable.rs
• crates/stdlib/src/_asyncio.rs
• crates/stdlib/src/socket.rs
• crates/vm/src/frame.rs
• crates/vm/src/gc_state.rs
• crates/vm/src/stdlib/gc.rs

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 475-518: found_in_inner_scope() currently examines sub_tables when
deciding to promote outer locals to Cells but doesn’t skip comprehension tables
that were inlined (st.comp_inlined), so closures get wrongly promoted after
we've already merged those symbols; update found_in_inner_scope() to ignore any
sub-table where st.comp_inlined is true (e.g., add a guard at the start of the
loop or conditional to continue if st.comp_inlined) so inlined comprehension
tables are not considered during closure promotion after merging via
analyze_symbol().

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 7de56247-53ae-4d7d-bd0d-ecea9530cad4

Reviewing files that changed from the base of the PR and between e58f796 and 1219da9.

• crates/codegen/src/compile.rs
• crates/codegen/src/symboltable.rs
• crates/vm/src/frame.rs

• crates/vm/src/frame.rs

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7662-7671: The inlined path is incorrectly used for async
comprehensions; modify the branch around current_code_info().in_inlined_comp and
compile_inlined_comprehension so that if has_an_async_gen is true you do NOT set
in_inlined_comp or call compile_inlined_comprehension (i.e., fall through to the
non-inlined comprehension code path that builds the AsyncComprehensionGenerator
and emits SetupFinally/END_ASYNC_FOR). Specifically, add a guard on
has_an_async_gen before calling compile_inlined_comprehension and only use the
inlined path when has_an_async_gen is false.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b992609c-f04d-4ac2-9caf-68edfb5b9e72

Reviewing files that changed from the base of the PR and between 1219da9 and 9121365.

• crates/codegen/src/compile.rs

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

718-740: ⚠️ Potential issue | 🔴 Critical

Skip inlined comprehensions during closure promotion.

The found_in_inner_scope() function still lacks a guard to skip comp_inlined tables. After merging inlined comprehension symbols into the parent (lines 475-518), the analysis at line 523 still passes the full sub_tables slice. An outer local referenced from an inlined comprehension will be incorrectly promoted to Cell, undermining the PEP 709 fast-local model.

Example: In def f(x): return [x + y for y in z], variable x should remain a fastlocal, not become a Cell.

,

Proposed fix

 fn found_in_inner_scope(
     &self,
     sub_tables: &[SymbolTable],
     name: &str,
     st_typ: CompilerScope,
 ) -> Option<SymbolScope> {
     sub_tables.iter().find_map(|st| {
+        if st.comp_inlined {
+            return None;
+        }
         let sym = st.symbols.get(name)?;
         if sym.scope == SymbolScope::Free || sym.flags.contains(SymbolFlags::FREE_CLASS) {

🤖 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 718 - 740, The function
found_in_inner_scope should skip inlined-comprehension symbol tables when
searching sub_tables to avoid promoting outer locals used only inside inlined
comprehensions; modify the iterator in found_in_inner_scope to ignore any st
where st.flags.contains(SymbolFlags::COMP_INLINED) (or the equivalent
comp_inlined marker on SymbolTable) before checking st.symbols.get(name), so
only real inner scopes are considered for Cell/GlobalExplicit decisions.

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 718-740: The function found_in_inner_scope should skip
inlined-comprehension symbol tables when searching sub_tables to avoid promoting
outer locals used only inside inlined comprehensions; modify the iterator in
found_in_inner_scope to ignore any st where
st.flags.contains(SymbolFlags::COMP_INLINED) (or the equivalent comp_inlined
marker on SymbolTable) before checking st.symbols.get(name), so only real inner
scopes are considered for Cell/GlobalExplicit decisions.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 15d44e4e-24eb-4383-831e-b6516b3b7586

Reviewing files that changed from the base of the PR and between 408255b and a6e3a80.

• crates/codegen/src/compile.rs
• crates/codegen/src/symboltable.rs