Walkthrough

Refactors dict construction and star-unpack compilation paths in the codegen, tightens runtime error messages for starred/mapping/unpack operations, and adds a function-descriptor helper used across frame and builtin error reporting.

Changes

Sequence Diagram(s)

sequenceDiagram
    autonumber
    Client->>Compiler: compile source with dict/** and *args usage
    note right of Compiler: emits bytecode sequences\n(BuildMap, DictUpdate, CallFunctionEx)
    Compiler->>VM: load bytecode
    Client->>VM: invoke function (runtime)
    VM/Frame->>VM/Frame: object_function_str(func) -> func_str
    VM/Frame->>Runtime: handle DictUpdate / iterate_mapping_keys using func_str
    alt mapping is invalid
        Runtime->>Client: raise TypeError("X argument after ** must be a mapping, not Y")
    else star-unpack single tuple-like
        VM->>Runtime: CallFunctionEx with compiled starred value
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Bump re to 3.14.2 #6809: Related changes to dict-unpacking and mapping validation in frame/iterate_mapping_keys and mapping-error wording.
• Align CallFunctionEx to 3.14 #6786: Related edits around CallFunctionEx and star/unpack argument emission in the compiler and VM.
• Replace custom opcodes with CPython standard sequences #6794: Related compile-time changes for dict/sequence construction and star-unpack opcode emission.

Suggested reviewers

• ShaharNaveh

Poem

🐰 I hopped through maps and starred arrays,

Rewrote the paths in tidy ways.
Now BuildMap sings when no ** plays,
And errors name the function’s days.
A little hop — the code behaves!

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• 📝 Generate docstrings

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

28-57: Mutating the caller's Vec via pop() is a hidden side-effect; consider a non-destructive approach.

format_missing_args takes &mut Vec solely to pop() the last element for formatting. This works because no caller reuses the vector afterwards, but the mutation is surprising and fragile. You can achieve the same with split_last() on a slice, avoiding the &mut requirement entirely.

♻️ Non-mutating alternative

-fn format_missing_args(
-    qualname: impl core::fmt::Display,
-    kind: &str,
-    missing: &mut Vec<impl core::fmt::Display>,
-) -> String {
-    let count = missing.len();
-    let last = if missing.len() > 1 {
-        missing.pop()
-    } else {
-        None
-    };
-    let (and, right): (&str, String) = if let Some(last) = last {
-        (
-            if missing.len() == 1 {
-                "' and '"
-            } else {
-                "', and '"
-            },
-            format!("{last}"),
-        )
-    } else {
-        ("", String::new())
-    };
-    format!(
-        "{qualname}() missing {count} required {kind} argument{}: '{}{}{right}'",
-        if count == 1 { "" } else { "s" },
-        missing.iter().join("', '"),
-        and,
-    )
-}
+fn format_missing_args(
+    qualname: impl core::fmt::Display,
+    kind: &str,
+    missing: &[impl core::fmt::Display],
+) -> String {
+    let count = missing.len();
+    let (init, and, right) = if count > 1 {
+        let (init, last) = missing.split_last().unwrap();
+        let and = if count == 2 { "' and '" } else { "', and '" };
+        (
+            core::slice::from_ref(last)
+                .iter()
+                .chain(init.iter().take(init.len().saturating_sub(0)))
+                .collect::<Vec<_>>(),
+            and,
+            format!("{last}"),
+        )
+    } else {
+        (missing.iter().collect(), "", String::new())
+    };
+    // Simpler: just use the original slice directly
+    let (init, and, right): (&[_], &str, String) = if let Some((last, rest)) = missing.split_last()
+        && rest.len() >= 1
+    {
+        (rest, if rest.len() == 1 { "' and '" } else { "', and '" }, format!("{last}"))
+    } else {
+        (missing, "", String::new())
+    };
+    format!(
+        "{qualname}() missing {count} required {kind} argument{}: '{}{}{right}'",
+        if count == 1 { "" } else { "s" },
+        init.iter().join("', '"),
+        and,
+    )
+}

Actually, let me simplify — the cleanest version using split_last:

♻️ Cleaner non-mutating version

 fn format_missing_args(
     qualname: impl core::fmt::Display,
     kind: &str,
-    missing: &mut Vec<impl core::fmt::Display>,
+    missing: &[impl core::fmt::Display],
 ) -> String {
     let count = missing.len();
-    let last = if missing.len() > 1 {
-        missing.pop()
-    } else {
-        None
-    };
-    let (and, right): (&str, String) = if let Some(last) = last {
-        (
-            if missing.len() == 1 {
-                "' and '"
-            } else {
-                "', and '"
-            },
-            format!("{last}"),
-        )
+    let (init, and, right): (&[_], &str, String) = if let Some((last, rest)) = missing.split_last()
+        && !rest.is_empty()
+    {
+        (rest, if rest.len() == 1 { "' and '" } else { "', and '" }, format!("{last}"))
     } else {
-        ("", String::new())
+        (missing, "", String::new())
     };
     format!(
         "{qualname}() missing {count} required {kind} argument{}: '{}{}{right}'",
         if count == 1 { "" } else { "s" },
-        missing.iter().join("', '"),
+        init.iter().join("', '"),
         and,
     )
 }

Then update call sites from &mut missing to &missing.

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

1073-1076: func_str is computed eagerly on every DictMerge, even on the happy path.

object_function_str performs get_attr("__qualname__") and get_attr("__module__") (potentially triggering Python-level __getattr__). This work is wasted when no error occurs, which is the common case. The same issue applies in collect_ex_args (lines 2612 and 2633).

Consider deferring the computation with a lazy closure or computing it only inside the error branches.

♻️ Sketch: defer func_str to error paths

-                let callable = self.nth_value(idx + 2);
-                let func_str = Self::object_function_str(callable, vm);
-
                 // Check if source is a mapping
                 if vm
                     .get_method(source.clone(), vm.ctx.intern_str("keys"))
                     .is_none()
                 {
+                    let callable = self.nth_value(idx + 2);
+                    let func_str = Self::object_function_str(callable, vm);
                     return Err(vm.new_type_error(format!(
                         "{} argument after ** must be a mapping, not {}",
                         func_str,

(Apply the same pattern for the duplicate-key branch and the two sites in collect_ex_args.)

---

2653-2676: Duplicated fallback logic — consider extracting the repr-or-"?" pattern.

Lines 2658–2661 and 2664–2667 are identical. A small helper or local closure would reduce duplication.

♻️ Suggested simplification

     fn object_function_str(obj: &PyObject, vm: &VirtualMachine) -> String {
+        let repr_fallback = || {
+            obj.repr(vm)
+                .map(|s| s.as_str().to_owned())
+                .unwrap_or_else(|_| "?".to_owned())
+        };
         let Ok(qualname) = obj.get_attr(vm.ctx.intern_str("__qualname__"), vm) else {
-            return obj
-                .repr(vm)
-                .map(|s| s.as_str().to_owned())
-                .unwrap_or_else(|_| "?".to_owned());
+            return repr_fallback();
         };
         let Some(qualname_str) = qualname.downcast_ref::<PyStr>() else {
-            return obj
-                .repr(vm)
-                .map(|s| s.as_str().to_owned())
-                .unwrap_or_else(|_| "?".to_owned());
+            return repr_fallback();
         };

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

---

1334-1337: Repeated non-iterability check pattern across 4 call sites.

The identical not_iterable guard appears at lines 1334–1337, 2634–2637, 2806–2809, and 3020–3023. Consider extracting a small helper method on PyObject or a free function:

fn is_not_iterable(obj: &PyObject, vm: &VirtualMachine) -> bool {
    obj.class().slots.iter.load().is_none()
        && obj
            .get_class_attr(vm.ctx.intern_str("__getitem__"))
            .is_none()
}

This makes the intent self-documenting and ensures future sites stay consistent.

• X
• Mastodon
• Reddit
• LinkedIn