heaptype __qualname__ #5848
heaptype __qualname__ #5848
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PyDict
participant DictInner
participant VM
User->>PyDict: pop_item(key, vm)
alt Exact dict
PyDict->>DictInner: remove_if_exists(key, vm)
DictInner-->>PyDict: Option<value>
PyDict-->>User: Option<value>
else Subclass/proxy
PyDict->>PyDict: get(key, vm)
PyDict->>PyDict: delitem(key, vm)
PyDict-->>User: Option<value>
end
sequenceDiagram
participant User
participant PyType
participant HeapTypeExt
User->>PyType: set __qualname__(value)
PyType->>HeapTypeExt: Store value in qualname field
User->>PyType: get __qualname__()
PyType->>HeapTypeExt: Read qualname field
HeapTypeExt-->>PyType: qualname or fallback to name
PyType-->>User: qualname
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (2)`Lib/**/*`: Files in the Lib/ directory (Python standard library copied from CPy...
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md) List of files the instruction was applied to:
`Lib/test/**/*`: Tests in Lib/test often use markers such as '# TODO: RUSTPYTHON...
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md) List of files the instruction was applied to:
🧠 Learnings (1)Lib/test/test_descr.py (1)undefined <retrieved_learning> 🧬 Code Graph Analysis (1)Lib/test/test_descr.py (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vm/src/builtins/type.rs (1)
869-875: Consider using expect instead of silent fallback.Since
__qualname__is already validated to be a string at lines 222-229, theand_thenwithdowncast_refsilently ignoring non-string values seems unnecessary. Consider usingexpectto make the invariant clear.let qualname = dict .pop_item(identifier!(vm, __qualname__).as_object(), vm)? - .and_then(|obj| obj.downcast_ref::<PyStr>().map(|s| s.to_owned())) - .or_else(|| { - // If __qualname__ is not provided, we can use the name as default - Some(name.clone()) - }); + .map(|obj| obj.downcast::<PyStr>().expect("__qualname__ already validated")) + .unwrap_or_else(|| name.clone());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Lib/test/test_typing.py(0 hunks)vm/src/builtins/dict.rs(1 hunks)vm/src/builtins/type.rs(6 hunks)vm/src/dict_inner.rs(2 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_typing.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
vm/src/builtins/type.rs (5)
vm/src/builtins/function.rs (7)
new(58-103)new(722-724)new(846-850)__qualname__(487-489)__qualname__(772-791)value(495-495)obj(449-449)vm/src/builtins/descriptor.rs (4)
new(34-44)__qualname__(117-119)__qualname__(252-262)qualname(235-235)vm/src/protocol/object.rs (1)
s(360-360)vm/src/builtins/builtin_func.rs (2)
__qualname__(87-107)__qualname__(155-160)vm/src/builtins/module.rs (2)
name(118-124)name(123-123)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (8)
vm/src/dict_inner.rs (5)
365-374: LGTM! Clean refactoring of the delete method.The change from checking a boolean to checking
is_some()maintains the same behavior while leveraging the new return type that provides the removed value.
376-381: Good backward compatibility wrapper.The method correctly maintains the existing API while delegating to the new
remove_if_existsmethod.
383-390: Well-designed conditional deletion method.The new
delete_ifmethod provides a clean API for conditional removal with predicate support.
391-396: Clean implementation of value-returning removal.The method correctly returns the removed value as
Option<T>, providing more information to callers compared to the boolean-returning variant.
400-419: Correct implementation of the refactored remove_if method.The method now returns the removed value, providing more useful information to callers while maintaining the same removal logic.
vm/src/builtins/dict.rs (1)
619-632: ```shell
#!/bin/bashSearch for other methods distinguishing exact vs non-exact dicts
rg -n "self\.exact_dict" -A2 -B2 vm/src/builtins/dict.rs
</details> <details> <summary>vm/src/builtins/type.rs (2)</summary> `61-68`: **Good addition of dedicated qualname storage.** The new `qualname` field in `HeapTypeExt` properly follows the same pattern as the `name` field, using `PyRwLock` for thread-safe access. Also applies to: 177-177 --- `582-594`: **Correct implementation of __qualname__ getter.** The getter properly returns the stored qualname for heap types with appropriate fallback to the type name, and returns the name directly for static types. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
fe81249 to
efb3d43
Compare
Summary by CodeRabbit
New Features
Noneif absent, without raising an error.Refactor
Tests