Fix dict unpack #5809
Fix dict unpack #5809
Conversation
WalkthroughThe updates improve Python dictionary unpacking and keyword argument handling in the RustPython VM. Dictionary unpacking now supports any mapping with a Changes
Sequence Diagram(s)sequenceDiagram
participant PythonUser
participant PythonVM
participant MappingObject
PythonUser->>PythonVM: Call function with **kwargs or dict unpacking
PythonVM->>MappingObject: Check for keys() method
alt keys() exists
PythonVM->>MappingObject: Call keys()
MappingObject-->>PythonVM: Return keys
loop For each key
PythonVM->>MappingObject: get_item(key)
alt key is string
PythonVM->>PythonVM: Add to kwargs/dict
else key is not string
PythonVM->>PythonUser: Raise TypeError (key not string)
end
end
else keys() missing
PythonVM->>PythonUser: Raise TypeError (not a mapping)
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the dictionary unpacking behavior to ensure that only mapping objects (i.e., those providing the keys() method) are accepted, and it adjusts kwargs collection accordingly. Key changes include:
- Adding a mapping protocol check before merging unpacked dictionaries.
- Refactoring kwargs extraction to use the keys() method in place of a direct dict downcast.
- Updating tests to verify error messages and order preservation, and clarifying documentation about modifying Python code.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vm/src/frame.rs | Fixes to dict unpacking and kwargs collection to enforce mapping protocol via keys() method. |
| extra_tests/snippets/builtin_dict.py | New tests for dict unpacking with non-mapping objects and order preservation in OrderedDict. |
| Lib/test/test_call.py | Removal of an expected failure marker for kwargs order, reflecting the implementation fix. |
| .github/copilot-instructions.md | Updated note to discourage editing Python code for bug fixes. |
vm/src/frame.rs
Outdated
| // 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::<PyStr>().is_none() { | ||
| return Err(vm.new_type_error("keywords must be strings".to_owned())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider abstracting the common pattern for retrieving mapping keys (via the keys() method) into a helper function to reduce code duplication and enhance maintainability.
Copilot uses AI. Check for mistakes.
vm/src/frame.rs
Outdated
| 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 { |
There was a problem hiding this comment.
Consider refactoring the repeated mapping keys retrieval logic (used both in dict unpacking and kwargs extraction) into a shared utility function for better code reuse.
Copilot uses AI. Check for mistakes.
aed3890 to
d973e6d
Compare
fix #5645
Summary by CodeRabbit
Bug Fixes
Documentation
Tests