dict: match CPython errors for invalid update sequence elements - #8338
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDict update-sequence processing now centralizes conversion, length validation, and contextual error handling for each element. ChangesDict update sequence handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_dict.py (TODO: 5) dependencies: dependent tests: (no tests depend on dict) Legend:
|
youknowone
left a comment
There was a problem hiding this comment.
other parts looks good, thank you!
There was a problem hiding this comment.
Pull request overview
This PR updates RustPython’s dict() / dict.update() sequence-merge path to more closely match CPython’s error reporting for invalid update-sequence elements, and enables the upstream Lib/test.test_dict.test_update_type_error by removing the RustPython-only expectedFailure.
Changes:
- Reworked
PyDict::merge_from_seq2to convert each update element into a sequence-like form first, so it can report the failing element index and its actual length when not exactly 2. - Added contextual exception notes for
TypeErrorfailures during element conversion/iteration. - Removed the
@unittest.expectedFailuremarker fromLib/test/test_dict.py::test_update_type_error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Lib/test/test_dict.py | Unmarks test_update_type_error as expected-failing now that the runtime behavior is implemented. |
| crates/vm/src/builtins/dict.rs | Implements CPython-aligned per-element validation (index + length) and contextual notes during update-from-seq2 merges, with list/tuple fast paths. |
Comments suppressed due to low confidence (1)
crates/vm/src/builtins/dict.rs:238
element.get_iter(vm)errors are currently rewritten toTypeError("object is not iterable")for allTypeErrors. This also rewritesTypeErrors raised by a user-defined__iter__(or other iterator construction paths), which contradicts the goal of preserving element-conversion/iterationTypeErrors and can change observable error messages.
Consider only rewriting the specific non-iterable case (i.e. when get_iter fails with the default "'' object is not iterable" message), and otherwise propagating the original TypeError unchanged (still adding the contextual note).
let elements = (|| {
let elem_iter = element.get_iter(vm).map_err(|exc| {
if exc.fast_isinstance(vm.ctx.exceptions.type_error) {
vm.new_type_error("object is not iterable")
} else {
exc
}
})?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn add_update_sequence_note(exc: &PyBaseExceptionRef, index: usize, vm: &VirtualMachine) { | ||
| if exc.fast_isinstance(vm.ctx.exceptions.type_error) { | ||
| let note = | ||
| format!("Cannot convert dictionary update sequence element #{index} to a sequence"); | ||
| let _ = vm.call_method(exc.as_object(), "add_note", (vm.ctx.new_str(note),)); | ||
| } | ||
| } |
youknowone
left a comment
There was a problem hiding this comment.
👍 Thank you for contributing! and welcome to RustPython project
| if exc.fast_isinstance(vm.ctx.exceptions.type_error) { | ||
| let note = | ||
| format!("Cannot convert dictionary update sequence element #{index} to a sequence"); | ||
| let _ = vm.call_method(exc.as_object(), "add_note", (vm.ctx.new_str(note),)); |
There was a problem hiding this comment.
ah, one more thing to check. is it correct to ignore the result of vm.call_method? don't we need to propagate this error?
If we don't need to propagate this error, please add a comment why let _ = is justified here.
There was a problem hiding this comment.
I used let _ = to ignore the error, following the existing add_note handling in type.rs and codecs.rs.
However, CPython propagates an error raised while adding the note and chains the original exception as its __context__.
I’ll update this to match CPython’s behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/vm/src/exceptions.rs:706
BaseException.add_note()currently raisesTypeError("Cannot add note: __notes__ is not a list")when__notes__exists but is not a list. CPython’s error message here is"__notes__ must be a list"; keeping the CPython wording helps compatibility and avoids surprising message diffs for code/tests that assert on it.
let notes = notes
.downcast::<PyList>()
.map_err(|_| vm.new_type_error("Cannot add note: __notes__ is not a list"))?;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/vm/src/builtins/dict.rs:244
- The
element.get_iter(vm)error mapping converts anyTypeErrorintoTypeError("object is not iterable"). That also overwritesTypeErrors raised by an element’s__iter__implementation (or other TypeErrors during iterator creation), which should be preserved (and only receive the contextual note). Consider only normalizing the specific “'T' object is not iterable” case produced byget_iterfor non-iterables, and otherwise returning the original exception.
let elem_iter = element.get_iter(vm).map_err(|exc| {
if exc.fast_isinstance(vm.ctx.exceptions.type_error) {
vm.new_type_error("object is not iterable")
} else {
exc
crates/vm/src/builtins/dict.rs:203
add_update_sequence_notecurrently returns theadd_notefailure (note_err) if adding the note fails. That can replace the originalTypeErrorfrom dict update conversion (e.g., if user code mutatesexc.__notes__to a non-list), contradicting the intent to preserve the original error and making note attachment change the raised exception. Other call sites treatadd_noteas best-effort and ignore failures (e.g.crates/vm/src/codecs.rs:376,crates/vm/src/builtins/type.rs:2520-2522).
This issue also appears on line 240 of the same file.
match vm.call_method(exc.as_object(), "add_note", (vm.ctx.new_str(note),)) {
Ok(_) => exc,
Err(note_err) => {
note_err.set___context__(Some(exc));
note_err
Assisted-by: Codex:gpt-5.6-Sol
Summary
Match CPython's error reporting when
dict()ordict.update()receives an invalid sequence element.test_update_type_error.Implementation details
Previously,
merge_from_seq2extracted the first two items and only checked whether a third item existed. As a result, it could only raise the generic error:CPython converts each dictionary update element to a sequence before validating its length. This allows it to report both the index of the failing element and its actual length:
This change follows the same approach:
PyListandPyTupleinstances use their underlying slices directly, avoiding an unnecessary allocation.Vecso that their actual length can be determined.TypeError: object is not iterable.TypeErrors raised while converting or iterating an element are preserved and receive the contextual note:PyListis imported to support the exact-list fast path, corresponding to CPython'sPySequence_Fasthandling of exact lists and tuples.Summary by CodeRabbit
__notes__exists but isn’t a list.