Fix dict.keys behavior #6476
Fix dict.keys behavior #6476
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/builtins/dict.rs (1)
66-107: Consider testing with proxy objects and edge cases.While the implementation is correct, the behavioral change should be thoroughly tested:
- Proxy objects: Verify that proxy objects with overridden
__getattribute__now correctly invoke thekeys()method- Non-callable keys attribute: Objects with a non-callable
keysattribute will now raiseTypeErrorinstead of falling back to iteration (this is more correct Python behavior)- Error propagation: Verify that errors raised during
keys()iteration are properly propagatedThese test scenarios will help ensure the fix works as intended for the reported issue.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/builtins/dict.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/builtins/dict.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (2)
crates/vm/src/builtins/dict.rs (2)
73-86: LGTM! Proper use ofget_attrfor Python attribute resolution semantics.The refactor correctly uses
get_attrto invoke__getattribute__, which is essential for proxy objects that override attribute access. The error handling is well-structured:
Ok(keys_method): Successfully calls the method and iterates over keysErr(AttributeError): Falls back to legacy iteration (no keys method)Err(other): Properly propagates errors (e.g., TypeError if keys is not callable, errors during iteration)This aligns with the PR objective to fix dict.keys behavior for proxy objects.
87-105: LGTM! Fallback logic correctly handles objects without keys method.The legacy iteration path properly handles objects that don't provide a
keys()method by:
- Iterating over the object directly
- Validating each element is a 2-tuple (key, value)
- Inserting each pair into the dictionary
The validation ensures exactly two elements per tuple, raising a clear error message otherwise.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.