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

1305-1320: Consider reusing PyFuture::make_cancelled_error_impl to reduce duplication.

This method is nearly identical to PyFuture::make_cancelled_error_impl. Since PyTask contains base: PyFuture, you could delegate to the base implementation:

♻️ Suggested refactor

 fn make_cancelled_error_impl(&self, vm: &VirtualMachine) -> PyBaseExceptionRef {
-    // If a saved CancelledError exists, take it (clearing the stored reference)
-    if let Some(exc) = self.base.fut_cancelled_exc.write().take()
-        && let Ok(exc) = exc.downcast::<PyBaseException>()
-    {
-        return exc;
-    }
-
-    let msg = self.base.fut_cancel_msg.read().clone();
-    let args = if let Some(m) = msg { vec![m] } else { vec![] };
-
-    match get_cancelled_error_type(vm) {
-        Ok(cancelled_error) => vm.new_exception(cancelled_error, args),
-        Err(_) => vm.new_runtime_error("cancelled"),
-    }
+    self.base.make_cancelled_error_impl(vm)
 }

🤖 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 1305 - 1320, The
PyTask::make_cancelled_error_impl implementation duplicates
PyFuture::make_cancelled_error_impl; replace the body to delegate to the base
PyFuture by returning self.base.make_cancelled_error_impl(vm) (adjust visibility
if needed so PyFuture::make_cancelled_error_impl is callable from PyTask),
keeping the same signature (fn make_cancelled_error_impl(&self, vm:
&VirtualMachine) -> PyBaseExceptionRef) and preserving behavior with
VirtualMachine/get_cancelled_error_type.

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

426-431: Consider extracting duplicated reset logic into a helper method.

The generation count reset pattern appears 4 times in collect_inner (lines 426-431, 539-542, 560-562, 707-710). Per the coding guidelines about avoiding duplicate code, consider extracting this into a helper method.

♻️ Suggested refactor

Add a helper method to GcState:

/// Reset generation counts after collection.
/// For gen2 (oldest), survivors stay in-place so don't reset gen2 count.
fn reset_generation_counts(&self, generation: usize) {
    let reset_end = if generation >= 2 { 2 } else { generation + 1 };
    for i in 0..reset_end {
        self.generations[i].count.store(0, Ordering::SeqCst);
    }
}

Then replace each occurrence with:

self.reset_generation_counts(generation);

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/vm/src/gc_state.rs` around lines 426 - 431, Extract the duplicated
generation-reset logic in collect_inner into a GcState helper method (e.g., fn
reset_generation_counts(&self, generation: usize)) that computes reset_end = if
generation >= 2 { 2 } else { generation + 1 } and loops over
self.generations[i].count.store(0, Ordering::SeqCst); then replace each repeated
block in collect_inner with a call to self.reset_generation_counts(generation)
so all four occurrences use the single helper.

crates/vm/src/stdlib/gc.rs (1)

202-210: Verify safety of frame pointer dereference.

The unsafe { fp.as_ref() } assumes fp is a valid NonNull<PyObject> (or similar) that is safe to dereference. While the frames are borrowed from vm.frames and should be valid during this scope, consider adding a brief safety comment explaining why this is sound.

🔧 Suggested safety comment

         let stack_frames: HashSet<usize> = vm
             .frames
             .borrow()
             .iter()
             .map(|fp| {
+                // SAFETY: Frame pointers in vm.frames are valid for the duration
+                // of the borrow, and we only read the pointer value.
                 let frame: &crate::PyObject = unsafe { fp.as_ref() }.as_ref();
                 frame as *const crate::PyObject as usize
             })
             .collect();

🤖 Prompt for AI Agents

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

In `@crates/vm/src/stdlib/gc.rs` around lines 202 - 210, The unsafe dereference
unsafe { fp.as_ref() } in the stack_frames construction must be annotated with a
short safety comment: explain that vm.frames is borrowed for the duration of
this scope, that it stores NonNull<crate::PyObject> (or equivalent raw pointers)
which are owned/kept alive by the VM/GC root set, and thus each fp is valid and
not concurrently mutated while borrowed—so calling fp.as_ref() and converting to
a usize is sound; add this comment immediately above the map closure referencing
vm.frames, fp.as_ref(), and stack_frames.

crates/vm/src/builtins/weakproxy.rs (1)

243-290: Please add regression coverage for the new numeric slot surface.

This now changes unary, binary, in-place, and ternary dispatch. A few focused cases for lhs/rhs weak proxies, dead proxies, and pow(proxy, x, mod) would make regressions much easier to catch.

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/weakproxy.rs` around lines 243 - 290, Add regression
tests that exercise the new numeric slot surface introduced in AS_NUMBER: cover
unary (proxy_unary_slot! handlers like _neg/_pos/_abs), binary
(proxy_binary_slot! handlers like _add/_sub/_mul), in-place (inplace_* handlers
such as _iadd/_imul) and ternary (proxy_ternary_slot! handlers like _pow/_ipow)
dispatch; specifically include tests where the weak proxy is the lhs and rhs of
operations, where the underlying referent has been dropped (dead proxy), and a
pow(proxy, x, mod) case to validate three-argument pow dispatch. Use the proxy
creation/upgrade utilities (proxy_upgrade and the weak proxy type in
weakproxy.rs) to construct proxies and drop targets to assert correct results or
TypeError/ReferenceError behavior so regressions in AS_NUMBER numeric slots are
detected.

crates/vm/src/protocol/object.rs (1)

373-379: Refactor ascii() to avoid duplicated branch wrapping.

The two branches only differ in value; return Ok(...) once after selecting the value.

♻️ Proposed refactor

 pub fn ascii(&self, vm: &VirtualMachine) -> PyResult<PyRef<PyStr>> {
     let repr = self.repr(vm)?;
-    if repr.as_wtf8().is_ascii() {
-        Ok(repr)
-    } else {
-        Ok(vm.ctx.new_str(to_ascii(repr.as_wtf8())))
-    }
+    let ascii_repr = if repr.as_wtf8().is_ascii() {
+        repr
+    } else {
+        vm.ctx.new_str(to_ascii(repr.as_wtf8()))
+    };
+    Ok(ascii_repr)
 }

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/vm/src/protocol/object.rs` around lines 373 - 379, The ascii() method
duplicates the Ok(...) wrapper across two branches; refactor by first obtaining
repr via self.repr(vm) and then compute a single PyRef<PyStr> result value: if
repr.as_wtf8().is_ascii() use repr, otherwise create
vm.ctx.new_str(to_ascii(repr.as_wtf8())); finally return Ok(result) once. Update
the function body around ascii(), repr(), to_ascii() and vm.ctx.new_str(...)
usage so only the differing value is chosen in the conditional and the Ok(...)
is returned a single time.

extra_tests/snippets/builtin_ascii.py (1)

24-27: Consider covering the %a formatting path too.

These assertions lock in ascii() itself, but the same PR also changed the %a/bytes conversion path in crates/vm/src/cformat.rs. One small formatting regression here would protect the cross-file behavior.

🤖 Prompt for AI Agents

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

In `@extra_tests/snippets/builtin_ascii.py` around lines 24 - 27, Add tests that
exercise the "%a" formatting path in addition to direct ascii() calls: for the
custom types Foo and Bar, assert that "%a" % Foo() produces the same MyStr-typed
result as ascii(Foo()) and that "%a" % Bar() yields the same byte-escaped string
as ascii(Bar()); also add the analogous bytes-format checks (e.g. b"%a" %
Foo()/Bar()) to cover the bytes conversion path touched in cformat.rs so the PR
protects cross-file behavior changes.

scripts/generate_opcode_metadata.py (1)

75-118: Consider adding error handling for missing deopt function.

The build_deopts function uses .group(1) on the regex search result without checking if the match succeeded. If the deopt function is not found in the source, this will raise an AttributeError.

♻️ Proposed fix

 def build_deopts(contents: str) -> dict[str, list[str]]:
-    raw_body = re.search(
+    match = re.search(
         r"fn deopt\(self\) -> Option<Self>(.*)", contents, re.DOTALL
-    ).group(1)
+    )
+    if not match:
+        return {}
+    raw_body = match.group(1)

🤖 Prompt for AI Agents

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

In `@scripts/generate_opcode_metadata.py` around lines 75 - 118, The build_deopts
function blindly calls .group(1) on the re.search result which will raise if the
deopt regex doesn't match; change it to capture the search result (e.g., m =
re.search(...)) and check if m is None before using m.group(1), and if missing
either return an empty dict or raise a clear ValueError/RuntimeError mentioning
the missing deopt function and include enough context (e.g., a snippet or
filename) to aid debugging; update build_deopts to use that guarded match
variable instead of calling .group(1) directly.

crates/vm/src/builtins/code.rs (2)

474-489: Simplify constant comparison with iterator.

The loop-based comparison can be replaced with a more idiomatic approach.

♻️ Proposed simplification

                 && {
                     let a_consts: Vec<_> = a.constants.iter().map(|c| c.0.clone()).collect();
                     let b_consts: Vec<_> = b.constants.iter().map(|c| c.0.clone()).collect();
-                    if a_consts.len() != b_consts.len() {
-                        false
-                    } else {
-                        let mut eq = true;
-                        for (ac, bc) in a_consts.iter().zip(b_consts.iter()) {
-                            if !vm.bool_eq(ac, bc)? {
-                                eq = false;
-                                break;
-                            }
-                        }
-                        eq
-                    }
+                    a_consts.len() == b_consts.len()
+                        && a_consts
+                            .iter()
+                            .zip(b_consts.iter())
+                            .try_fold(true, |_, (ac, bc)| {
+                                vm.bool_eq(ac, bc).map(|eq| eq)
+                            })?
                 };

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/code.rs` around lines 474 - 489, Replace the manual
loop that compares a_consts and b_consts with an iterator-based check: after
collecting a.constants and b.constants (used in a_consts and b_consts) and
verifying lengths match, use an iterator zip over
a_consts.iter().zip(b_consts.iter()) combined with a fallible iterator adapter
such as try_for_each or try_fold to call vm.bool_eq(ac, bc)? for each pair and
aggregate to a single boolean (true only if all pairs are equal), returning
early on error; update the block that currently sets and returns eq to this
iterator-based approach while keeping the existing length check and using the
same vm.bool_eq call.

---

495-517: Hash implementation creates temporary objects on every call.

The hash implementation allocates a new tuple containing multiple new Python objects (strings, ints, bytes, nested tuple) for every hash computation. This could be expensive for repeated hashing operations (e.g., using code objects as dict keys).

Consider caching the hash value in PyCode (lazy computation with atomic storage), similar to how Python strings cache their hash. Alternatively, compute the hash directly from the fields without creating Python objects.

♻️ Example of direct hash computation

impl Hashable for PyCode {
    fn hash(zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<crate::common::hash::PyHash> {
        use crate::common::hash;
        use std::hash::{Hash, Hasher};
        
        let code = &zelf.code;
        let mut hasher = hash::HashSecret::new();
        
        code.obj_name.as_str().hash(&mut hasher);
        code.arg_count.hash(&mut hasher);
        code.posonlyarg_count.hash(&mut hasher);
        code.kwonlyarg_count.hash(&mut hasher);
        code.varnames.len().hash(&mut hasher);
        code.flags.bits().hash(&mut hasher);
        code.first_line_number.map_or(0, |n| n.get()).hash(&mut hasher);
        code.instructions.original_bytes().hash(&mut hasher);
        // Constants need special handling for Python equality semantics
        
        Ok(hash::fix_sentinel(hasher.finish() as i64))
    }
}

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/code.rs` around lines 495 - 517, The current
Hashable::hash impl for PyCode allocates many temporary Python objects
(vm.ctx.new_* and a tuple) on every call; change it to avoid allocations by
either (A) adding a cached, lazily-computed atomic hash field to the PyCode
struct and computing/storing it once in PyCode::hash, or (B) computing the hash
directly from the Rust fields without creating Python objects: use
crate::common::hash::HashSecret (or the project's hashing utilities) and feed
code.obj_name.as_str(), code.arg_count, code.posonlyarg_count,
code.kwonlyarg_count, code.varnames.len(), code.flags.bits(),
code.first_line_number.map_or(0, …), and code.instructions.original_bytes() into
the hasher, handle code.constants with the same semantics as equality (or
include their precomputed hashes), then return
crate::common::hash::fix_sentinel(hasher.finish() as i64) as the PyHash;
implement atomic lazy init if choosing caching to be thread-safe.

crates/compiler-core/src/bytecode.rs (1)

321-335: Blanket Index impl on [T] may cause conflicts.

Implementing Index<oparg::VarNum> for [T] is a blanket implementation on a standard library type. While this works within this crate, it could lead to coherence issues or unexpected behavior if other crates also implement similar traits. The TODO comments correctly note that a newtype wrapper for CodeObject.varnames would be the proper solution.

Consider prioritizing the newtype approach to avoid potential issues as the codebase grows.

🤖 Prompt for AI Agents

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

In `@crates/compiler-core/src/bytecode.rs` around lines 321 - 335, Remove the
blanket impls of Index/IndexMut for [T] that take oparg::VarNum and instead
introduce a newtype wrapper for the CodeObject.varnames field (e.g., a
Varnames<T> or VarNames(Vec<T>/Box<[T]>) type) and move the Index/IndexMut
implementations onto that newtype so only the newtype can be indexed by
oparg::VarNum; implement Index/IndexMut for Varnames<T> using var_num.as_usize()
to access the underlying storage and update CodeObject.varnames to use the new
type.

crates/stdlib/src/_tokenize.rs (3)

640-644: Consider using a named struct instead of tuple for pending token parts.

The 6-element tuple (u8, String, usize, usize, usize, usize) is hard to reason about. A named struct would improve readability and maintainability:

struct PendingTokenPart {
    token_type: u8,
    text: String,
    start_line: usize,
    start_col: usize,
    end_line: usize,
    end_col: usize,
}

🤖 Prompt for AI Agents

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

In `@crates/stdlib/src/_tokenize.rs` around lines 640 - 644, Replace the opaque
6-tuple type used in pending_fstring_parts with a named struct to improve
clarity: define a struct (e.g., PendingTokenPart) with fields token_type: u8,
text: String, start_line: usize, start_col: usize, end_line: usize, end_col:
usize and update the pending_fstring_parts field to Vec<PendingTokenPart>;
update any places that construct, deconstruct, or index into the tuple to use
the struct field names (also keep pending_empty_fstring_middle as-is unless you
want a parallel refactor) so callers use PendingTokenPart.token_type, .text,
.start_line, etc., instead of tuple positions.

---

107-147: Potential race condition in state update pattern.

The state is cloned on line 109, modified locally, then written back on line 138. If the iterator is accessed concurrently (e.g., via threading with released GIL), modifications between the read and write could be lost. Consider using a single write lock throughout, or document that concurrent access is unsupported.

♻️ Alternative using write lock throughout

     impl IterNext for PyTokenizerIter {
         fn next(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyIterReturn> {
-            let mut state = zelf.state.read().clone();
+            let mut state_guard = zelf.state.write();

             loop {
-                match &mut state.phase {
+                match &mut state_guard.phase {
                     TokenizerPhase::Reading { source } => {
                         let line = zelf.readline(vm)?;
                         if line.is_empty() {
                             let accumulated = core::mem::take(source);
                             // ... rest of logic
-                            state.phase = TokenizerPhase::Yielding { ... };
+                            state_guard.phase = TokenizerPhase::Yielding { ... };
                         } else {
                             source.push_str(&line);
                         }
                     }
                     TokenizerPhase::Yielding { .. } => {
-                        let result = emit_next_token(&mut state, zelf.extra_tokens, vm)?;
-                        *zelf.state.write() = state;
+                        let result = emit_next_token(&mut state_guard, zelf.extra_tokens, vm)?;
                         return Ok(result);
                     }
                     // ...
                 }
             }
         }
     }

🤖 Prompt for AI Agents

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

In `@crates/stdlib/src/_tokenize.rs` around lines 107 - 147, The next method on
IterNext for PyTokenizerIter clones the state via zelf.state.read().clone() and
later writes it back, which can lose concurrent updates; change to acquire a
write lock once (use zelf.state.write()) and mutate the state in place while
calling readline, updating TokenizerPhase variants, and invoking emit_next_token
so modifications are atomic; alternatively, document that concurrent iterator
access is unsupported if you prefer not to change locking, but do not retain the
read-clone-write pattern used with zelf.state.read()/clone() and write().

---

248-256: Repeated source.lines().count() calls could be cached.

source.lines().count() is called multiple times throughout the function (lines 248, 375, 386). Since source doesn't change during the Yielding phase, this value could be computed once and stored in the state.

🤖 Prompt for AI Agents

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

In `@crates/stdlib/src/_tokenize.rs` around lines 248 - 256, Compute
source.lines().count() once and reuse it instead of calling it multiple times:
introduce a local variable (e.g., total_lines or last_line) near the start of
the function or before the Yielding phase and replace all occurrences of
source.lines().count() (used to compute last_line/default_pos at the block with
extra_tokens and the later uses around lines referenced) with that cached
variable; ensure existing logic that builds default_pos and the call to
next_non_dedent_info(tokens, *index, source, line_index, default_pos) uses the
cached value so behavior is unchanged.

crates/compiler-core/src/bytecode/instruction.rs (1)

1231-1247: Extract a shared formatter for the paired fast-local disassembly paths.

These arms now unpack the same VarNums shape in three places, and STORE_FAST_LOAD_FAST has already drifted by printing raw indexes instead of resolved var names. A tiny helper that takes the opcode label would keep the output consistent and avoid future divergence.

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: 1354-1369

🤖 Prompt for AI Agents

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

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1231 - 1247,
The two arms (Self::LoadFastLoadFast and Self::LoadFastBorrowLoadFastBorrow)
(and the similar STORE_FAST_LOAD_FAST case) duplicate the same logic of fetching
var_nums.get(arg), calling oparg.indexes(), resolving varname(idx) for both
indexes and writing the formatted string; extract a small helper function (e.g.,
format_paired_vars or fmt_paired_fast_locals) that takes the opcode label
string, the formatter f, &var_nums and arg, performs var_nums.get(arg) ->
oparg.indexes() -> varname(idx1/idx2) and does the write! call, then replace the
duplicated blocks to call that helper to keep output consistent and avoid
divergence.