Skip to content

dict: match CPython errors for invalid update sequence elements#8338

Open
2jiyong wants to merge 1 commit into
RustPython:mainfrom
2jiyong:fix-dict-update-type-error
Open

dict: match CPython errors for invalid update sequence elements#8338
2jiyong wants to merge 1 commit into
RustPython:mainfrom
2jiyong:fix-dict-update-type-error

Conversation

@2jiyong

@2jiyong 2jiyong commented Jul 20, 2026

Copy link
Copy Markdown

Assisted-by: Codex:gpt-5.6-Sol

Summary

Match CPython's error reporting when dict() or dict.update() receives an invalid sequence element.

  • Add a contextual exception note when an element cannot be converted to a sequence, enabling test_update_type_error.
  • Report the zero-based element index and actual sequence length when an element does not contain exactly two items.

Implementation details

Previously, merge_from_seq2 extracted the first two items and only checked whether a third item existed. As a result, it could only raise the generic error:

Iterator must have exactly two elements

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:

dictionary update sequence element #0 has length 3; 2 is required

This change follows the same approach:

  • Exact PyList and PyTuple instances use their underlying slices directly, avoiding an unnecessary allocation.
  • Other iterable objects are collected into a Vec so that their actual length can be determined.
  • Non-iterable elements raise TypeError: object is not iterable.
  • TypeErrors raised while converting or iterating an element are preserved and receive the contextual note:
Cannot convert dictionary update sequence element #0 to a sequence

PyList is imported to support the exact-list fast path, corresponding to CPython's PySequence_Fast handling of exact lists and tuples.

Summary by CodeRabbit

  • Bug Fixes
    • Improved dictionary updates from sequences by consistently validating that each item contains exactly two elements.
    • Improved error messages for invalid or non-iterable update items, including contextual details identifying the problematic item.
    • Added optimized handling for list- and tuple-based update entries while preserving support for other iterables.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: ff2695dd-3b29-49fe-b7e1-1b5e14a76f65

📥 Commits

Reviewing files that changed from the base of the PR and between cc30cd5 and 0aed904.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_dict.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/builtins/dict.rs

📝 Walkthrough

Walkthrough

Dict update-sequence processing now centralizes conversion, length validation, and contextual error handling for each element. merge_from_seq2 enumerates input elements and delegates key-value pair parsing to the new helpers.

Changes

Dict update sequence handling

Layer / File(s) Summary
Centralized update-element parsing
crates/vm/src/builtins/dict.rs
Adds list and tuple fast paths, generic iterable handling, exact two-element validation, and contextual TypeError notes.
Merge integration
crates/vm/src/builtins/dict.rs
Updates merge_from_seq2 to enumerate elements and use the centralized pair parser.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: bschoenmaeckers, shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: aligning dict update errors with CPython for invalid update sequence elements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_dict.py (TODO: 5)
[x] test: cpython/Lib/test/test_dictcomps.py (TODO: 1)
[x] test: cpython/Lib/test/test_dictviews.py (TODO: 1)
[x] test: cpython/Lib/test/test_userdict.py
[x] test: cpython/Lib/test/mapping_tests.py

dependencies:

dependent tests: (no tests depend on dict)

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone
youknowone requested a review from moreal July 22, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant