_abc, _typing and update typing,test_types from 3.14.2 #6797
Conversation
📝 WalkthroughWalkthroughThis PR standardizes binary operator hooks to return PyResult, reworks PyUnion to track hashable vs unhashable components with dedup/flatten logic, adds a new stdlib Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant make_union as make_union()
participant dedup as dedup_and_flatten_args()
participant PyUnion as PyUnion
Caller->>make_union: call with args (PyTuple)
make_union->>dedup: dedup_and_flatten_args(args)
dedup->>dedup: flatten strings -> ForwardRef, separate hashable/unhashable, dedupe
dedup-->>make_union: UnionComponents {args, hashable_args?, unhashable_args?}
alt single arg
make_union-->>Caller: return underlying arg (unwrapped)
else multiple args
make_union->>PyUnion: from_components(UnionComponents)
PyUnion-->>make_union: PyUnion instance (PyResult)
make_union-->>Caller: PyUnion (PyResult)
end
sequenceDiagram
participant Code
participant ABCMethods as _abc methods
participant Cache as Registry/Cache
participant Counter as InvalidationCounter
Code->>ABCMethods: _abc_subclasscheck(cls, subclass)
ABCMethods->>Cache: check positive cache
alt cached hit
Cache-->>ABCMethods: result
else cache miss
ABCMethods->>Cache: check negative_cache version
alt negative cache stale
ABCMethods->>Counter: increment token
ABCMethods->>Cache: clear negative cache
end
ABCMethods->>ABCMethods: run subclass hooks / direct checks / recursive registry traversal
ABCMethods->>Cache: update positive or negative cache
end
ABCMethods-->>Code: boolean result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin abc |
youknowone
force-pushed
the
abc
branch
3 times, most recently
from
6df2b89 to
e8077f2
Compare
youknowone
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/union.rs`:
- Around line 248-332: The dedup_and_flatten_args function currently treats any
error from arg.hash(vm) as "unhashable", masking real exceptions; change the
Err(_) arm to capture the exception (Err(e)) and propagate it unless it's a
TypeError, in which case proceed with unhashable handling. Concretely, in
dedup_and_flatten_args replace the match on arg.hash(vm) so that if Err(e) you
check the exception type (compare against vm.ctx.exceptions.type_error or use
vm.is_instance) and return Err(e) for non-TypeError, otherwise handle as now by
doing equality comparisons and adding to unhashable_list.
🧹 Nitpick comments (2)
crates/vm/src/stdlib/_abc.rs (1)
341-350: Avoid calling into the VM while holdingAbcDatalocks.Drop the lock guard before invoking
set.clear()to reduce reentrancy/deadlock risk; apply the same pattern in_reset_registryand_reset_caches.♻️ Suggested refactor pattern (apply similarly in reset helpers)
- let negative_cache = impl_data.negative_cache.read(); - if let Some(ref set) = *negative_cache { - vm.call_method(set.as_ref(), "clear", ())?; - } - drop(negative_cache); + let negative_cache = impl_data.negative_cache.read(); + let set = negative_cache.as_ref().cloned(); + drop(negative_cache); + if let Some(set) = set { + vm.call_method(set.as_ref(), "clear", ())?; + } impl_data.set_cache_version(invalidation_counter);Also applies to: 453-477
crates/vm/src/builtins/union.rs (1)
389-441: Optional: fast equality path when all args are hashable.If both unions have
unhashable_args == None, consider comparinghashable_args(frozensets) directly to avoid the O(n²) nested loops.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/namespace.rs`:
- Around line 46-69: The __replace__ implementation currently calls cls.call((),
vm) then mutates the new instance's __dict__; instead, build a mapping from the
current namespace (src_dict) and call the constructor with that mapping as the
single positional argument and the replacement kwargs (args.kwargs) as **kwargs
so subclasses' __init__ runs normally. Concretely, stop creating result then
copying src_dict and calling set_attr; instead prepare a positional tuple
containing the namespace mapping and pass args.kwargs into cls.call(...) so
cls.call receives (mapping,) and the kwargs, preserving existing checks for
positional args and using the same vm/context objects (referencing __replace__,
cls.call, zelf, src_dict, and args.kwargs).
♻️ Duplicate comments (1)
crates/vm/src/builtins/union.rs (1)
290-307: Hash error handling catches all exceptions.The
Err(_)pattern treats any hash exception as "unhashable," including non-TypeErrorexceptions likeValueError. While this could mask real errors from faulty__hash__implementations, I note that a similar concern was raised previously and the maintainer indicated the current behavior is intentional.
🧹 Nitpick comments (1)
crates/vm/src/builtins/union.rs (1)
458-492: Minor: Potentially misleading error message when previously-unhashable args become hashable.Lines 470-474 raise
"union contains N unhashable element(s)"even when all previously-unhashable args have since become hashable (passedarg.hash(vm)?at line 466). This maintains consistent hashing semantics (objects that were unhashable at creation time remain treated as such), but the error message could be confusing since the elements are no longer actually unhashable.Consider updating the message to something like:
"union was created with unhashable element(s)"to clarify the semantic.Suggested message improvement
- return Err(vm.new_type_error(format!( - "union contains {} unhashable element{}", - n, - if n > 1 { "s" } else { "" } - ))); + return Err(vm.new_type_error(format!( + "union was created with {} unhashable element{}", + n, + if n > 1 { "s" } else { "" } + )));
| #[pymethod] | ||
| fn __replace__(zelf: PyObjectRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult { | ||
| if !args.args.is_empty() { | ||
| return Err(vm.new_type_error("__replace__() takes no positional arguments")); | ||
| } | ||
|
|
||
| // Create a new instance of the same type | ||
| let cls: PyObjectRef = zelf.class().to_owned().into(); | ||
| let result = cls.call((), vm)?; | ||
|
|
||
| // Copy the current namespace dict to the new instance | ||
| let src_dict = zelf.dict().unwrap(); | ||
| let dst_dict = result.dict().unwrap(); | ||
| for (key, value) in src_dict { | ||
| dst_dict.set_item(&*key, value, vm)?; | ||
| } | ||
|
|
||
| // Update with the provided kwargs | ||
| for (name, value) in args.kwargs { | ||
| let name = vm.ctx.new_str(name); | ||
| result.set_attr(&name, value, vm)?; | ||
| } | ||
|
|
||
| Ok(result) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython 3.14 SimpleNamespace.__replace__ implementation behavior kwargs constructor
💡 Result:
Short answer: SimpleNamespace.replace (used by copy.replace()) creates a new instance by calling the SimpleNamespace constructor with the current namespace contents as the initial mapping/positional argument and applying the replace keyword arguments on top, so replacement kwargs override existing keys. See the SimpleNamespace constructor docs (mapping_or_iterable=(), /, **kwargs) and the copy.replace description in the 3.13+ changes. [1][2]
Sources
- types.SimpleNamespace docs (constructor behavior: optional positional mapping/iterable then **kwargs; kwargs update afterwards). [1]
- What's new / copy.replace description (copy.replace uses replace; SimpleNamespace is supported). [2]
Fix replace to pass attributes through constructor to match CPython behavior.
Line 54 calls cls.call((), vm) with no arguments, then manually copies the namespace dict and applies kwargs via set_attr. This bypasses the constructor and doesn't match CPython 3.14 behavior. In CPython, SimpleNamespace.__replace__ passes the current namespace contents as a positional mapping argument and the replacement kwargs as **kwargs to the constructor. This allows subclasses with __init__ validation or custom logic to properly process the merged attributes. Pass the current namespace dict as a positional argument and the replacement kwargs to cls.call() instead of constructing an empty instance and mutating __dict__ afterward.
🤖 Prompt for AI Agents
In `@crates/vm/src/builtins/namespace.rs` around lines 46 - 69, The __replace__
implementation currently calls cls.call((), vm) then mutates the new instance's
__dict__; instead, build a mapping from the current namespace (src_dict) and
call the constructor with that mapping as the single positional argument and the
replacement kwargs (args.kwargs) as **kwargs so subclasses' __init__ runs
normally. Concretely, stop creating result then copying src_dict and calling
set_attr; instead prepare a positional tuple containing the namespace mapping
and pass args.kwargs into cls.call(...) so cls.call receives (mapping,) and the
kwargs, preserving existing checks for positional args and using the same
vm/context objects (referencing __replace__, cls.call, zelf, src_dict, and
args.kwargs).
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython SimpleNamespace __replace__ source code implementation
💡 Result:
The SimpleNamespace replace implementation is in CPython’s C source: see Objects/namespaceobject.c in the CPython repository (types.SimpleNamespace is implemented there). The copy.replace protocol and that SimpleNamespace supports it are documented in the copy module docs and the types docs. References:
- CPython source (namespaceobject.c). [1]
- copy.replace / replace documentation. [2]
- types.SimpleNamespace (docs noting support for copy.replace). [3]
References
- Objects/namespaceobject.c — CPython source (types.SimpleNamespace implementation). [turn9view0].
- Doc: copy.replace and object.replace (copy module docs). [turn3search3].
- Doc: types.SimpleNamespace (notes it’s supported by copy.replace). [turn10search0].
🌐 Web query:
CPython namespaceobject.c __replace__ method implementation details
💡 Result:
Short answer: In CPython the SimpleNamespace replace is implemented in C (Objects/namespaceobject.c). It accepts keyword arguments, copies the namespace's dict, updates that copy with the provided keyword changes, constructs and returns a new SimpleNamespace whose dict is the updated dict (leaving the original instance unchanged). This is the mechanism used to support the new copy.replace()/object.replace protocol added for Python 3.13. See the Python 3.13 notes and the checkin adding object.replace documentation. [1][2]
References
- Python 3.13 "copy.replace" / object.replace description. [1]
- Python checkins (Doc/library/copy.rst change mentioning object.replace, Python 3.13). [2]
🏁 Script executed:
#!/bin/bash
# Fetch the actual CPython source code for SimpleNamespace __replace__ implementation
curl -s "https://raw.githubusercontent.com/python/cpython/v3.14.2/Objects/namespaceobject.c" | grep -A 50 "__replace__" | head -60
Length of output: 2424
@youknowone, you're absolutely right—I apologize for the incorrect suggestion.
I've now reviewed the actual CPython 3.14.2 source code from Objects/namespaceobject.c. The CPython implementation:
- Rejects positional arguments with
_PyArg_NoPositional - Creates a new empty instance via
PyObject_CallNoArgs((PyObject *)Py_TYPE(self)) - Updates the new instance's
__dict__from the original instance viaPyDict_Update - Updates the new instance's
__dict__with the kwargs if provided
Your Rust implementation correctly mirrors this behavior by calling cls.call((), vm) with no arguments, then copying the dict and applying kwargs. The claim that CPython passes namespace contents as a positional argument to the constructor was wrong.
✏️ Learnings added
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6797
File: crates/vm/src/builtins/namespace.rs:46-69
Timestamp: 2026-01-20T02:02:14.639Z
Learning: In RustPython's SimpleNamespace implementation (crates/vm/src/builtins/namespace.rs), the __replace__ method correctly creates a new empty instance via cls.call((), vm) then mutates its dict, matching CPython's pattern of calling PyObject_CallNoArgs then updating __dict__ directly. Do not suggest passing existing namespace contents as positional arguments to the constructor.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
The last commit is generated by #6796
python3 scripts/update_lib quick typingSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.