Reuse stored hashes in dict.fromkeys() - #8503
Conversation
📝 WalkthroughWalkthrough
ChangesHash-aware
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: [ ] test: cpython/Lib/test/test_set.py (TODO: 3) dependencies: dependent tests: (no tests depend on set) Legend:
|
Closes RustPython#8490. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fe77067 to
324ecaa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/builtins/dict.rs (1)
401-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both paths.
Use a key type that counts
__hash__calls. After creating exactdict,set, andfrozensetsources, verify thatdict.fromkeysperforms no additional key hashing. Also test dictionary and set subclasses that override__iter__; these inputs must use the generic path.This verifies the stated PR objective for cached hashes and subclass iteration behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/builtins/dict.rs` around lines 401 - 410, Add regression tests covering both branches in dict.fromkeys: with exact dict, set, and frozenset inputs, use a key type counting __hash__ calls and verify no additional hashing occurs; with dictionary and set subclasses overriding __iter__, verify the generic iteration path is used. Anchor the tests to the dict.fromkeys behavior implemented around fromkeys_known_hashes and preserve the expected cached-hash and subclass-iteration semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/vm/src/builtins/dict.rs`:
- Around line 401-410: Add regression tests covering both branches in
dict.fromkeys: with exact dict, set, and frozenset inputs, use a key type
counting __hash__ calls and verify no additional hashing occurs; with dictionary
and set subclasses overriding __iter__, verify the generic iteration path is
used. Anchor the tests to the dict.fromkeys behavior implemented around
fromkeys_known_hashes and preserve the expected cached-hash and
subclass-iteration semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a217c01-a5f7-4d77-969d-0817ee391d15
⛔ Files ignored due to path filters (1)
Lib/test/test_set.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/builtins/dict.rscrates/vm/src/builtins/set.rs
3 participants
Summary
dict.fromkeys() recomputed hash for every key even when the source already stored a hash per entry. CPython's _PyDict_FromKeys branches on PyDict_CheckExact / PyAnySet_CheckExact and feeds the hash read from the source table straight into insertdict; RustPython always iterated generically through setitem.
The checks are the exact ones CPython uses: a set or dict subclass may override iter, so reading its table directly would change what the call observes. exact_set_keys_with_hashes() is therefore separate from extract_set(), which stays subclass-inclusive for the set operations.
Summary by CodeRabbit
Performance
dict.fromkeyswhen creating dictionaries from existing dictionaries, sets, or frozensets.Compatibility