Merge pull request #5809 from youknowone/fix-dict-unpack

youknowone · web-flow · commit efc6b5dbaf98 · 2025-06-22T09:08:06.000+09:00
Fix dict unpack
diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md
@@ -102,6 +102,7 @@ Run `./whats_left.py` to get a list of unimplemented methods, which is helpful w
 
 ### Python Code
 
+- **IMPORTANT**: In most cases, Python code should not be edited. Bug fixes should be made through Rust code modifications only
 - Follow PEP 8 style for custom Python code
 - Use ruff for linting Python code
 - Minimize modifications to CPython standard library files
diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py
@@ -13,8 +13,6 @@
 
 class FunctionCalls(unittest.TestCase):
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_kwargs_order(self):
         # bpo-34320:  **kwargs should preserve order of passed OrderedDict
         od = collections.OrderedDict([('a', 1), ('b', 2)])
diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py
@@ -358,3 +358,53 @@ def __eq__(self, other):
 assert it.__length_hint__() == 0
 assert_raises(StopIteration, next, it)
 assert it.__length_hint__() == 0
+
+# Test dictionary unpacking with non-mapping objects
+# This should raise TypeError for non-mapping objects
+with assert_raises(TypeError) as cm:
+    {**[1, 2]}
+assert "'list' object is not a mapping" in str(cm.exception)
+
+with assert_raises(TypeError) as cm:
+    {**[[1, 2], [3, 4]]}
+assert "'list' object is not a mapping" in str(cm.exception)
+
+with assert_raises(TypeError) as cm:
+    {**"string"}
+assert "'str' object is not a mapping" in str(cm.exception)
+
+with assert_raises(TypeError) as cm:
+    {**(1, 2, 3)}
+assert "'tuple' object is not a mapping" in str(cm.exception)
+
+# Test that valid mappings still work
+assert {**{"a": 1}, **{"b": 2}} == {"a": 1, "b": 2}
+
+# Test OrderedDict unpacking preserves order
+import collections
+
+od = collections.OrderedDict([("a", 1), ("b", 2)])
+od.move_to_end("a")  # Move 'a' to end: ['b', 'a']
+expected_order = list(od.items())  # [('b', 2), ('a', 1)]
+
+
+def test_func(**kwargs):
+    return kwargs
+
+
+result = test_func(**od)
+assert list(result.items()) == expected_order, (
+    f"Expected {expected_order}, got {list(result.items())}"
+)
+
+# Test multiple OrderedDict unpacking
+od1 = collections.OrderedDict([("x", 10), ("y", 20)])
+od2 = collections.OrderedDict([("z", 30), ("w", 40)])
+od2.move_to_end("z")  # Move 'z' to end: ['w', 'z']
+
+result = test_func(**od1, **od2)
+# Should preserve order: x, y, w, z
+expected_keys = ["x", "y", "w", "z"]
+assert list(result.keys()) == expected_keys, (
+    f"Expected {expected_keys}, got {list(result.keys())}"
+)
diff --git a/vm/src/frame.rs b/vm/src/frame.rs
@@ -796,6 +796,19 @@ impl ExecutingFrame<'_> {
                     .top_value()
                     .downcast_ref::<PyDict>()
                     .expect("exact dict expected");
+
+                // For dictionary unpacking {**x}, x must be a mapping
+                // Check if the object has the mapping protocol (keys method)
+                if vm
+                    .get_method(other.clone(), vm.ctx.intern_str("keys"))
+                    .is_none()
+                {
+                    return Err(vm.new_type_error(format!(
+                        "'{}' object is not a mapping",
+                        other.class().name()
+                    )));
+                }
+
                 dict.merge_object(other, vm)?;
                 Ok(None)
             }
@@ -1527,11 +1540,12 @@ impl ExecutingFrame<'_> {
         let size = size as usize;
         let map_obj = vm.ctx.new_dict();
         for obj in self.pop_multiple(size) {
-            // Take all key-value pairs from the dict:
-            let dict: PyDictRef = obj.downcast().map_err(|obj| {
-                vm.new_type_error(format!("'{}' object is not a mapping", obj.class().name()))
-            })?;
-            for (key, value) in dict {
+            // Use keys() method for all mapping objects to preserve order
+            Self::iterate_mapping_keys(vm, &obj, "keyword argument", |key| {
+                // Check for keyword argument restrictions
+                if key.downcast_ref::<PyStr>().is_none() {
+                    return Err(vm.new_type_error("keywords must be strings".to_owned()));
+                }
                 if map_obj.contains_key(&*key, vm) {
                     let key_repr = &key.repr(vm)?;
                     let msg = format!(
@@ -1540,8 +1554,11 @@ impl ExecutingFrame<'_> {
                     );
                     return Err(vm.new_type_error(msg));
                 }
+
+                let value = obj.get_item(&*key, vm)?;
                 map_obj.set_item(&*key, value, vm)?;
-            }
+                Ok(())
+            })?;
         }
 
         self.push_value(map_obj.into());
@@ -1586,17 +1603,18 @@ impl ExecutingFrame<'_> {
 
     fn collect_ex_args(&mut self, vm: &VirtualMachine, has_kwargs: bool) -> PyResult<FuncArgs> {
         let kwargs = if has_kwargs {
-            let kw_dict: PyDictRef = self.pop_value().downcast().map_err(|_| {
-                // TODO: check collections.abc.Mapping
-                vm.new_type_error("Kwargs must be a dict.".to_owned())
-            })?;
+            let kw_obj = self.pop_value();
             let mut kwargs = IndexMap::new();
-            for (key, value) in kw_dict.into_iter() {
-                let key = key
+
+            // Use keys() method for all mapping objects to preserve order
+            Self::iterate_mapping_keys(vm, &kw_obj, "argument after **", |key| {
+                let key_str = key
                     .payload_if_subclass::<PyStr>(vm)
                     .ok_or_else(|| vm.new_type_error("keywords must be strings".to_owned()))?;
-                kwargs.insert(key.as_str().to_owned(), value);
-            }
+                let value = kw_obj.get_item(&*key, vm)?;
+                kwargs.insert(key_str.as_str().to_owned(), value);
+                Ok(())
+            })?;
             kwargs
         } else {
             IndexMap::new()
@@ -1608,6 +1626,28 @@ impl ExecutingFrame<'_> {
         Ok(FuncArgs { args, kwargs })
     }
 
+    /// Helper function to iterate over mapping keys using the keys() method.
+    /// This ensures proper order preservation for OrderedDict and other custom mappings.
+    fn iterate_mapping_keys<F>(
+        vm: &VirtualMachine,
+        mapping: &PyObjectRef,
+        error_prefix: &str,
+        mut key_handler: F,
+    ) -> PyResult<()>
+    where
+        F: FnMut(PyObjectRef) -> PyResult<()>,
+    {
+        let Some(keys_method) = vm.get_method(mapping.clone(), vm.ctx.intern_str("keys")) else {
+            return Err(vm.new_type_error(format!("{} must be a mapping", error_prefix)));
+        };
+
+        let keys = keys_method?.call((), vm)?.get_iter(vm)?;
+        while let PyIterReturn::Return(key) = keys.next(vm)? {
+            key_handler(key)?;
+        }
+        Ok(())
+    }
+
     #[inline]
     fn execute_call(&mut self, args: FuncArgs, vm: &VirtualMachine) -> FrameResult {
         let func_ref = self.pop_value();
