Skip to content

Fix dict unpack #5809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)])
Expand Down
50 changes: 50 additions & 0 deletions extra_tests/snippets/builtin_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())}"
)
68 changes: 54 additions & 14 deletions vm/src/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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!(
Expand All @@ -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());
Expand Down Expand Up @@ -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()
Expand All @@ -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();
Expand Down
Loading