From 829388b7b5aeefa4968b9434a9326304865a6b56 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 21 Jun 2025 22:00:01 +0900 Subject: [PATCH 1/4] Edit only Rust files --- .github/copilot-instructions.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b6e85b9540..53636f0230 100644 --- 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 From 9467632e1b3965bb453f9a69faea7f4adba21fac Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 21 Jun 2025 23:47:15 +0900 Subject: [PATCH 2/4] Fix dict unpacking order preservation --- Lib/test/test_call.py | 2 -- vm/src/frame.rs | 41 +++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index 8e64ffffd0..3cb9659acb 100644 --- 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/vm/src/frame.rs b/vm/src/frame.rs index 54f740e883..29598c75e6 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1527,11 +1527,20 @@ 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 + let Some(keys_method) = vm.get_method(obj.clone(), vm.ctx.intern_str("keys")) else { + return Err( + vm.new_type_error(format!("'{}' object is not a mapping", obj.class().name())) + ); + }; + + let keys = keys_method?.call((), vm)?.get_iter(vm)?; + while let PyIterReturn::Return(key) = keys.next(vm)? { + // Check for keyword argument restrictions + if key.downcast_ref::().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,6 +1549,8 @@ impl ExecutingFrame<'_> { ); return Err(vm.new_type_error(msg)); } + + let value = obj.get_item(&*key, vm)?; map_obj.set_item(&*key, value, vm)?; } } @@ -1586,16 +1597,22 @@ impl ExecutingFrame<'_> { fn collect_ex_args(&mut self, vm: &VirtualMachine, has_kwargs: bool) -> PyResult { 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 + let Some(keys_method) = vm.get_method(kw_obj.clone(), vm.ctx.intern_str("keys")) else { + return Err(vm.new_type_error("argument after ** must be a mapping".to_owned())); + }; + + // Handle custom mapping objects like OrderedDict using keys() method + let keys = keys_method?.call((), vm)?.get_iter(vm)?; + while let PyIterReturn::Return(key) = keys.next(vm)? { + let key_str = key .payload_if_subclass::(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); } kwargs } else { From 3f1d39e5db3660e6d72a9d490d31d0f4bbb65fc4 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 21 Jun 2025 23:53:17 +0900 Subject: [PATCH 3/4] Fix DictUpdate validation --- extra_tests/snippets/builtin_dict.py | 50 ++++++++++++++++++++++++++++ vm/src/frame.rs | 13 ++++++++ 2 files changed, 63 insertions(+) diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index abd93539a5..83a8c5f994 100644 --- 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 index 29598c75e6..3c9766bde1 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -796,6 +796,19 @@ impl ExecutingFrame<'_> { .top_value() .downcast_ref::() .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) } From d973e6da10abff7c486ac1cbe9e5427813ea0c64 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sun, 22 Jun 2025 00:01:22 +0900 Subject: [PATCH 4/4] Apply copilot review --- vm/src/frame.rs | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 3c9766bde1..6539a5a390 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1541,19 +1541,11 @@ impl ExecutingFrame<'_> { let map_obj = vm.ctx.new_dict(); for obj in self.pop_multiple(size) { // Use keys() method for all mapping objects to preserve order - let Some(keys_method) = vm.get_method(obj.clone(), vm.ctx.intern_str("keys")) else { - return Err( - vm.new_type_error(format!("'{}' object is not a mapping", obj.class().name())) - ); - }; - - let keys = keys_method?.call((), vm)?.get_iter(vm)?; - while let PyIterReturn::Return(key) = keys.next(vm)? { + Self::iterate_mapping_keys(vm, &obj, "keyword argument", |key| { // Check for keyword argument restrictions if key.downcast_ref::().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!( @@ -1565,7 +1557,8 @@ impl ExecutingFrame<'_> { let value = obj.get_item(&*key, vm)?; map_obj.set_item(&*key, value, vm)?; - } + Ok(()) + })?; } self.push_value(map_obj.into()); @@ -1614,19 +1607,14 @@ impl ExecutingFrame<'_> { let mut kwargs = IndexMap::new(); // Use keys() method for all mapping objects to preserve order - let Some(keys_method) = vm.get_method(kw_obj.clone(), vm.ctx.intern_str("keys")) else { - return Err(vm.new_type_error("argument after ** must be a mapping".to_owned())); - }; - - // Handle custom mapping objects like OrderedDict using keys() method - let keys = keys_method?.call((), vm)?.get_iter(vm)?; - while let PyIterReturn::Return(key) = keys.next(vm)? { + Self::iterate_mapping_keys(vm, &kw_obj, "argument after **", |key| { let key_str = key .payload_if_subclass::(vm) .ok_or_else(|| vm.new_type_error("keywords must be strings".to_owned()))?; let value = kw_obj.get_item(&*key, vm)?; kwargs.insert(key_str.as_str().to_owned(), value); - } + Ok(()) + })?; kwargs } else { IndexMap::new() @@ -1638,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( + 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();