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

3086-3116: Nit: tuple/list type checks on Lines 3092-3093 are unreachable in the general path.

Exact tuple and list values are handled by the fast path above (lines 3022-3034) and never reach this code. Only dict_type can match here. The dead checks are harmless but slightly misleading.

Simplify to only check dict_type

                 let cls = value.class();
-                let msg = if cls.is(vm.ctx.types.list_type)
-                    || cls.is(vm.ctx.types.tuple_type)
-                    || cls.is(vm.ctx.types.dict_type)
-                {
+                let msg = if cls.is(vm.ctx.types.dict_type) {

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 3086 - 3116, The branch handling
PyIterReturn::Return contains unreachable checks for list/tuple types because
those are handled earlier in the fast path; update the logic in the return
branch by removing the list_type and tuple_type checks and only test
cls.is(vm.ctx.types.dict_type) when deciding whether to attempt value.length(vm)
and include "got N" in the message; ensure you keep the same fallback message
generation (format!("too many values to unpack (expected {size})") and
vm.new_value_error(msg)) so behavior for non-dict iterables remains unchanged
(refer to iter.next(vm), PyIterReturn::Return, value.class(), cls.is(...),
value.length(vm), size, and vm.new_value_error).

---

3035-3035: Nit: let _ = cls; is a no-op.

cls is a reference (no destructor), and it was already consumed by the if/else if checks above, so this line has no effect — it neither drops a resource nor suppresses an unused-variable warning.

Proposed fix

-        let _ = cls;
-
-        if let Some(elements) = fast_elements {
+        if let Some(elements) = fast_elements {

🤖 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 3035, The statement `let _ = cls;` in
frame.rs is a no-op because `cls` is a reference already consumed by the
preceding `if`/`else if` branches; remove that line to avoid misleading/no-op
code. If the intent was to silence an unused-variable warning, instead prefix
the binding with an underscore where `cls` is declared or explicitly call
`drop(cls)` (if ownership needs to be released); otherwise simply delete `let _
= cls;` near the end of the matching `if`/`else if` block that references `cls`.

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

3018-3051: Avoid cloning full tuple/list when lengths mismatch.
The fast path currently clones the entire container just to compare lengths; for large mismatches this defeats the optimization. Consider checking length first and only cloning/pushing when equal.

♻️ Suggested refactor

-        let cls = value.class();
-        let fast_elements: Option<Vec<PyObjectRef>> = if cls.is(vm.ctx.types.tuple_type) {
-            Some(value.downcast_ref::<PyTuple>().unwrap().as_slice().to_vec())
-        } else if cls.is(vm.ctx.types.list_type) {
-            Some(
-                value
-                    .downcast_ref::<PyList>()
-                    .unwrap()
-                    .borrow_vec()
-                    .to_vec(),
-            )
-        } else {
-            None
-        };
-        if let Some(elements) = fast_elements {
-            return match elements.len().cmp(&size) {
+        let cls = value.class();
+        if cls.is(vm.ctx.types.tuple_type) {
+            let tuple = value.downcast_ref::<PyTuple>().unwrap();
+            return match tuple.len().cmp(&size) {
                 core::cmp::Ordering::Equal => {
                     self.state
                         .stack
-                        .extend(elements.into_iter().rev().map(Some));
+                        .extend(tuple.as_slice().iter().rev().cloned().map(Some));
                     Ok(None)
                 }
                 core::cmp::Ordering::Greater => Err(vm.new_value_error(format!(
                     "too many values to unpack (expected {size}, got {})",
-                    elements.len()
+                    tuple.len()
                 ))),
                 core::cmp::Ordering::Less => Err(vm.new_value_error(format!(
                     "not enough values to unpack (expected {size}, got {})",
-                    elements.len()
+                    tuple.len()
                 ))),
             };
-        }
+        } else if cls.is(vm.ctx.types.list_type) {
+            let list = value.downcast_ref::<PyList>().unwrap();
+            let elements = list.borrow_vec();
+            return match elements.len().cmp(&size) {
+                core::cmp::Ordering::Equal => {
+                    self.state
+                        .stack
+                        .extend(elements.iter().rev().cloned().map(Some));
+                    Ok(None)
+                }
+                core::cmp::Ordering::Greater => Err(vm.new_value_error(format!(
+                    "too many values to unpack (expected {size}, got {})",
+                    elements.len()
+                ))),
+                core::cmp::Ordering::Less => Err(vm.new_value_error(format!(
+                    "not enough values to unpack (expected {size}, got {})",
+                    elements.len()
+                ))),
+            };
+        }

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 3018 - 3051, The fast-path in the unpack
sequence logic (the cls/fast_elements branch) currently clones the whole
tuple/list via to_vec()/to_vec() before comparing lengths; change it to first
check length from the concrete type (e.g., call PyTuple::len or
PyList::borrow_vec().len or an equivalent length accessor on value.class checks)
and only allocate/clone the elements when the length equals size, then push them
onto self.state.stack (preserving the existing reverse+Some mapping). Update the
branch that handles Ordering::Equal to perform the cloning/extend, and make the
Greater/Less arms return the error without allocating.

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

3022-3051: Consider checking length before cloning elements to avoid unnecessary allocations on error paths.

When the length doesn't match size, the cloned Vec is never used. You could compare the length first (via as_slice().len() / borrow_vec().len()) and only clone when equal, avoiding allocation + clone overhead for the mismatch cases.

♻️ Sketch

-        let fast_elements: Option<Vec<PyObjectRef>> = if cls.is(vm.ctx.types.tuple_type) {
-            Some(value.downcast_ref::<PyTuple>().unwrap().as_slice().to_vec())
-        } else if cls.is(vm.ctx.types.list_type) {
-            Some(
-                value
-                    .downcast_ref::<PyList>()
-                    .unwrap()
-                    .borrow_vec()
-                    .to_vec(),
-            )
-        } else {
-            None
-        };
-        if let Some(elements) = fast_elements {
-            return match elements.len().cmp(&size) {
-                core::cmp::Ordering::Equal => {
-                    self.state
-                        .stack
-                        .extend(elements.into_iter().rev().map(Some));
-                    Ok(None)
-                }
-                core::cmp::Ordering::Greater => Err(vm.new_value_error(format!(
-                    "too many values to unpack (expected {size}, got {})",
-                    elements.len()
-                ))),
-                core::cmp::Ordering::Less => Err(vm.new_value_error(format!(
-                    "not enough values to unpack (expected {size}, got {})",
-                    elements.len()
-                ))),
-            };
-        }
+        if cls.is(vm.ctx.types.tuple_type) {
+            let slice = value.downcast_ref::<PyTuple>().unwrap().as_slice();
+            let got = slice.len();
+            return match got.cmp(&size) {
+                core::cmp::Ordering::Equal => {
+                    self.state.stack.extend(slice.iter().rev().cloned().map(Some));
+                    Ok(None)
+                }
+                core::cmp::Ordering::Greater => Err(vm.new_value_error(format!(
+                    "too many values to unpack (expected {size}, got {got})"
+                ))),
+                core::cmp::Ordering::Less => Err(vm.new_value_error(format!(
+                    "not enough values to unpack (expected {size}, got {got})"
+                ))),
+            };
+        } else if cls.is(vm.ctx.types.list_type) {
+            let borrowed = value.downcast_ref::<PyList>().unwrap().borrow_vec();
+            let got = borrowed.len();
+            return match got.cmp(&size) {
+                core::cmp::Ordering::Equal => {
+                    self.state.stack.extend(borrowed.iter().rev().cloned().map(Some));
+                    Ok(None)
+                }
+                core::cmp::Ordering::Greater => Err(vm.new_value_error(format!(
+                    "too many values to unpack (expected {size}, got {got})"
+                ))),
+                core::cmp::Ordering::Less => Err(vm.new_value_error(format!(
+                    "not enough values to unpack (expected {size}, got {got})"
+                ))),
+            };
+        }

This also avoids the intermediate Option<Vec<_>> and the extra indirection. For tuples, you can iterate the slice directly without allocating a Vec at all.

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 3022 - 3051, The current code eagerly
clones tuples/lists into a Vec in the `fast_elements` branch even when lengths
don't match; instead first check the sequence length (for tuple use
`as_slice().len()`, for list use `borrow_vec().len()`), compare it to `size`,
return the appropriate `Vm` error when lengths differ, and only then clone or
iterate elements when lengths are equal; replace the `Option<Vec<_>>` pattern by
branching on `cls.is(...)` to obtain the length, and when equal either iterate
the tuple slice (no allocation) or clone the list via `to_vec()` and then call
`self.state.stack.extend(... .rev().map(Some))` as before.

---

3086-3103: Simplify nested branches — three of four produce the same message.

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

♻️ Proposed simplification

-                let msg = if value.class().is(vm.ctx.types.dict_type) {
-                    if let Ok(got) = value.length(vm) {
-                        if got > size {
-                            format!("too many values to unpack (expected {size}, got {got})")
-                        } else {
-                            format!("too many values to unpack (expected {size})")
-                        }
-                    } else {
-                        format!("too many values to unpack (expected {size})")
-                    }
-                } else {
-                    format!("too many values to unpack (expected {size})")
-                };
+                let got = if value.class().is(vm.ctx.types.dict_type) {
+                    value.length(vm).ok().filter(|&n| n > size)
+                } else {
+                    None
+                };
+                let msg = if let Some(got) = got {
+                    format!("too many values to unpack (expected {size}, got {got})")
+                } else {
+                    format!("too many values to unpack (expected {size})")
+                };

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 3086 - 3103, The nested branches in the
PyIterReturn::Return arm duplicate the same error text; refactor by first
computing an Option<usize> named e.g. got_opt using
value.class().is(vm.ctx.types.dict_type) then value.length(vm) (map success to
Some(got) else None), and then build the msg once: if let Some(got) = got_opt
and got > size => "too many values...got {got}", else "too many
values...expected {size}" (or the variant without got). Finally return
Err(vm.new_value_error(msg)). Update the code in the PyIterReturn::Return block
to use the common message construction instead of the four nested format
branches.

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

3084-3111: Dict-specific branch in the exhaustion check: the got <= size fallback is unreachable for non-mutated dicts.

On line 3092, if got > size should always be true for a non-concurrently-mutated exact dict (we already pulled size items and got one more), making the else on line 3094 a dead branch under normal conditions. This is acceptable as defensive coding against concurrent dict mutation during iteration, but consider simplifying to always include got in the message when value.length(vm) succeeds, which is what CPython does:

Suggested simplification

                 let msg = if value.class().is(vm.ctx.types.dict_type) {
                     if let Ok(got) = value.length(vm) {
-                        if got > size {
-                            format!("too many values to unpack (expected {size}, got {got})")
-                        } else {
-                            format!("too many values to unpack (expected {size})")
-                        }
+                        format!("too many values to unpack (expected {size}, got {got})")
                     } else {
                         format!("too many values to unpack (expected {size})")
                     }

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 3084 - 3111, The dict-specific branch
builds an error message that omits the actual dict size when value.length(vm)
succeeds, creating an unreachable else for non-mutated dicts; update the code in
the exhaustion check (the match handling PyIterReturn::Return in frame.rs around
iter.next(vm)) so that when value.class().is(vm.ctx.types.dict_type) and
value.length(vm) returns Ok(got) you always include "got {got}" in the message
(i.e., format!("too many values to unpack (expected {size}, got {got})")), and
only fall back to the shorter message when value.length(vm) returns an Err,
leaving the other branches unchanged.