Repository navigation
Update weakref from v3.14.2 - #6899
Conversation
📝 WalkthroughWalkthroughThis pull request refactors argument handling for Python's WeakRef constructor by introducing a dedicated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: [x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (146 tests)
Legend:
|
![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/weakref.rs`:
- Around line 44-58: slot_new currently ignores keyword args but must reject
them like CPython 3.14; in the slot_new function check args.kwargs and if not
empty return Err(vm.new_type_error("ref() takes no keyword
arguments".to_owned())); place this check before consuming positional args
(before referent.downgrade_with_typ) so calls like weakref.ref(obj, callback=cb)
raise a TypeError, matching CPython behavior and other builtins (use
args.kwargs, vm.new_type_error, and keep existing logic in slot_new intact).
🧹 Nitpick comments (1)
crates/vm/src/builtins/weakref.rs (1)
17-24: Consider narrowingWeakNewArgsvisibility and avoid blanketallow(dead_code).If this type isn’t used outside the module,
pub(crate)(or removingpub) keeps the public API smaller, and dropping the lint suppression avoids masking real unused warnings.♻️ Suggested tweak
-#[allow(dead_code)] -pub struct WeakNewArgs { +pub(crate) struct WeakNewArgs {
| fn slot_new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult { | ||
| let Self::Args { referent, callback } = args.bind(vm)?; | ||
| let weak = referent.downgrade_with_typ(callback.into_option(), cls, vm)?; | ||
| // PyArg_UnpackTuple: only process positional args, ignore kwargs. | ||
| // Subclass __init__ will handle extra kwargs. | ||
| let mut positional = args.args.into_iter(); | ||
| let referent = positional.next().ok_or_else(|| { | ||
| vm.new_type_error("__new__ expected at least 1 argument, got 0".to_owned()) | ||
| })?; | ||
| let callback = positional.next(); | ||
| if let Some(_extra) = positional.next() { | ||
| return Err(vm.new_type_error(format!( | ||
| "__new__ expected at most 2 arguments, got {}", | ||
| 3 + positional.count() | ||
| ))); | ||
| } | ||
| let weak = referent.downgrade_with_typ(callback, cls, vm)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file and verify its existence
find . -name "weakref.rs" -type fRepository: RustPython/RustPython
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check Rust formatting and linting per guidelines
cd crates/vm && cargo fmt -- --check && echo "✓ Formatting OK" || echo "✗ Formatting issues found"Repository: RustPython/RustPython
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Run clippy with strict warnings
cd crates/vm && cargo clippy --all-targets --all-features -D warnings 2>&1 | head -100Repository: RustPython/RustPython
Length of output: 604
🌐 Web query:
CPython 3.14 weakref.ref kwargs behavior - does it accept keyword arguments or raise TypeError
💡 Result:
In CPython 3.14, weakref.ref does not accept keyword arguments. Passing any **kwargs (including callback=) raises TypeError (e.g. “ref() takes no keyword arguments”). You must pass the optional callback positionally:
import weakref
r = weakref.ref(obj, cb) # OK
r = weakref.ref(obj, callback=cb) # TypeError on CPython 3.14The documented constructor form is weakref.ref(object[, callback]) (positional parameters). [1]
Sources:
[1] https://docs.python.org/3/library/weakref.html
🏁 Script executed:
#!/bin/bash
# Examine the weakref.rs file to understand kwargs handling
cat -n crates/vm/src/builtins/weakref.rs | head -80Repository: RustPython/RustPython
Length of output: 2932
🏁 Script executed:
#!/bin/bash
# Search for kwargs handling in weakref implementations
rg "args\.kwargs" crates/vm/src/builtins/weakref.rs -A 3 -B 3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there's any validation/guard for kwargs in the slot_new context
rg "slot_new|FuncArgs|kwargs" crates/vm/src/builtins/weakref.rs -A 2 -B 2Repository: RustPython/RustPython
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Look at the broader context of how Constructor trait handles kwargs
rg "impl Constructor" crates/vm/src/builtins/weakref.rs -A 20Repository: RustPython/RustPython
Length of output: 918
🏁 Script executed:
#!/bin/bash
# Check the FuncArgs structure to understand how kwargs are passed
fd -e rs -e toml . crates/vm/src | xargs rg "struct FuncArgs" -A 5Repository: RustPython/RustPython
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Look for how Constructor trait slot_new is invoked - check if kwargs validation happens before slot_new
rg "trait Constructor" crates/vm/src -A 10 | head -40Repository: RustPython/RustPython
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Search for how Constructor.slot_new is called in the framework
rg "slot_new" crates/vm/src -B 3 -A 1 | grep -A 5 "Constructor"Repository: RustPython/RustPython
Length of output: 12384
🏁 Script executed:
#!/bin/bash
# Check how other builtins handle kwargs validation in slot_new
rg "fn slot_new.*FuncArgs" crates/vm/src/builtins -A 8 | head -60Repository: RustPython/RustPython
Length of output: 4347
🏁 Script executed:
#!/bin/bash
# Specifically check if any other builtin rejects kwargs explicitly
rg "args\.kwargs" crates/vm/src/builtins -B 5 -A 3 | head -80Repository: RustPython/RustPython
Length of output: 5365
Add explicit kwargs validation to reject keyword arguments, matching CPython 3.14 behavior.
CPython 3.14's weakref.ref() rejects all keyword arguments with TypeError ("ref() takes no keyword arguments"), but the current slot_new silently ignores kwargs without validation. Add an explicit check:
if !args.kwargs.is_empty() {
return Err(vm.new_type_error("ref() takes no keyword arguments".to_owned()))
}This ensures compatibility with CPython's strict positional-only semantics and prevents silent acceptance of invalid calls like weakref.ref(obj, callback=cb). The pattern is used consistently in other builtins (e.g., PyBaseException, PyTemplate).
Note: Rust formatting/linting verification failed due to environment setup errors (LZMA extraction failures). Please recheck cargo fmt and cargo clippy when tools are available.
🤖 Prompt for AI Agents
In `@crates/vm/src/builtins/weakref.rs` around lines 44 - 58, slot_new currently
ignores keyword args but must reject them like CPython 3.14; in the slot_new
function check args.kwargs and if not empty return Err(vm.new_type_error("ref()
takes no keyword arguments".to_owned())); place this check before consuming
positional args (before referent.downgrade_with_typ) so calls like
weakref.ref(obj, callback=cb) raise a TypeError, matching CPython behavior and
other builtins (use args.kwargs, vm.new_type_error, and keep existing logic in
slot_new intact).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.