Add per-type vectorcall for builtin constructors - #7407
Merged
youknowone merged 1 commit intoMar 18, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 175c07c7-2428-4690-997c-b0fa19917c4f 📥 CommitsReviewing files that changed from the base of the PR and between f76e0d1 and ff165a2. ⛔ Files ignored due to path filters (3)
📝 Walkthrough WalkthroughThe PR makes targeted adjustments to error handling, Windows API behavior, and type instantiation control. Changes include reorienting mmap error reporting to use OS errors, handling empty Windows environment mappings with proper NUL termination, removing deprecated WinSock error codes, and preventing instantiation of the Windows version class through a TypeError slot. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem🚥 Pre-merge checks | ✅ 1 | ❌ 4 ❌ Failed checks (4 warnings)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 1) dependencies: dependent tests: (2 tests)
[x] lib: cpython/Lib/subprocess.py dependencies:
dependent tests: (81 tests)
[x] test: cpython/Lib/test/test_sys.py (TODO: 7) dependencies: dependent tests: (225 tests)
Legend:
|
Sorry, something went wrong.
youknowone
force-pushed
the
vectorcall
branch
2 times, most recently
from
2b764a2 to
a9ee2e3
Compare
March 17, 2026 11:38
youknowone
marked this pull request as ready for review
March 17, 2026 11:40
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)crates/vm/src/builtins/float.rs (1)🤖 Prompt for all review comments with AI agents528-538: Extract a shared vectorcall-to-slot_new helper to avoid drift.
Line 528-Line 538 duplicates the same trampoline used by multiple builtins. Centralizing this in one helper would keep behavior consistent and lower maintenance risk.
Proposed refactor sketch+// e.g. in a shared builtins/type helper module +pub(crate) fn vectorcall_via_slot_new( + zelf_obj: &PyObject, + args: Vec<PyObjectRef>, + nargs: usize, + kwnames: Option<&[PyObjectRef]>, + vm: &VirtualMachine, +) -> PyResult { + let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap(); + let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames); + (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args, vm) +}Then each builtin can just store that helper in its slots.vectorcall.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/float.rs` around lines 528 - 538, The vectorcall_float trampoline duplicates the pattern of converting vectorcall args to FuncArgs and invoking the type's slot_new (see vectorcall_float, FuncArgs::from_vectorcall_owned, and (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args, vm)); extract that logic into a single reusable helper (e.g., a function that accepts zelf_obj, args, nargs, kwnames, vm and performs the downcast, FuncArgs conversion, and calls slot_new) and have builtins set their slots.vectorcall to point to this helper to avoid drift and ensure consistent behavior across types.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/builtins/dict.rs`: - Around line 1438-1449: The vectorcall_dict fast-path constructs a base dict with PyDict::default().into_ref_with_type(...) and PyDict::slot_init(...) without invoking the type's constructor hooks, so it must detect when the receiver type has custom new/init hooks and fall back to the generic PyType::call path; update vectorcall_dict to check the target type's slots (e.g. inspect zelf.slots.new or zelf.slots.init or whether the type is exactly dict) and if non-default hooks exist (or the type != built-in dict) call PyType::call(zelf_obj, args, vm) (or the equivalent generic constructor path) instead of the fast default+slot_init; apply the same pattern to the analogous fast-paths in list.rs and bytearray.rs to preserve proper subclass constructor semantics. In `@crates/vm/src/builtins/tuple.rs`: - Around line 698-709: The vectorcall_tuple implementation uses the type's slot_new but returns immediately, skipping any subtype slot_init; update vectorcall_tuple to mirror PyType::call by invoking the type's slots.new (zelf.slots.new) to create the instance, then if the type has a non-None slots.init (zelf.slots.init.load().is_some()) call that init slot with the newly created object and the original FuncArgs (func_args) and vm before returning the instance; do the same fix in the bytes vectorcall implementation to ensure Rust-defined subtypes receive their post-new initializer. In `@crates/vm/src/builtins/type.rs`: - Around line 2668-2674: The branch currently dispatches an inherited vectorcall when zelf.slots.call is None but doesn't check whether slots.new allows instantiation; update the condition that calls type_vc so it also requires zelf.slots.new.load().is_some() (i.e., only run type_vc(zelf_obj, args, nargs, kwnames, vm) when vectorcall is Some AND slots.new is Some) to prevent dispatching constructors for DISALLOW_INSTANTIATION types; alternatively, when implementing set_new() for DISALLOW_INSTANTIATION ensure it clears slots.vectorcall alongside setting slots.new to None so the invariant (no vectorcall if no new) holds. In `@crates/vm/src/types/slot.rs`: - Around line 828-833: The inherit_from_mro handling for SlotAccessor::TpInit (and TpNew) must also restore the vectorcall slot when the primary slot is deleted: mirror the TpCall logic by copying the ancestor's vectorcall into self.slots.vectorcall in the non-ADD (deletion) path so heap types regain fast-path calls; locate the TpInit/TpNew cases (where update_main_slot! is used for init/init_wrapper/Init) and add the same vectorcall restoration that TpCall performs in inherit_from_mro. --- Nitpick comments: In `@crates/vm/src/builtins/float.rs`: - Around line 528-538: The vectorcall_float trampoline duplicates the pattern of converting vectorcall args to FuncArgs and invoking the type's slot_new (see vectorcall_float, FuncArgs::from_vectorcall_owned, and (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args, vm)); extract that logic into a single reusable helper (e.g., a function that accepts zelf_obj, args, nargs, kwnames, vm and performs the downcast, FuncArgs conversion, and calls slot_new) and have builtins set their slots.vectorcall to point to this helper to avoid drift and ensure consistent behavior across types.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 81cce507-bbb7-45af-85bc-bfd1b573eedf
📥 CommitsReviewing files that changed from the base of the PR and between 2ea5960 and a9ee2e3.
📒 Files selected for processing (14)- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/bytearray.rs
- crates/vm/src/builtins/bytes.rs
- crates/vm/src/builtins/complex.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/builtins/float.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/set.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/types/slot.rs
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/dict.rs
Comment on lines
+1438
to
+1449
| fn vectorcall_dict( | ||
| zelf_obj: &PyObject, | ||
| args: Vec<PyObjectRef>, | ||
| nargs: usize, | ||
| kwnames: Option<&[PyObjectRef]>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap(); | ||
| let obj = PyDict::default().into_ref_with_type(vm, zelf.to_owned())?; | ||
| let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames); | ||
| PyDict::slot_init(obj.clone().into(), func_args, vm)?; | ||
| Ok(obj.into()) |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
This fast path hardwires base dict construction.
PyDict::default().into_ref_with_type(...) plus PyDict::slot_init(...) never consults zelf.slots.new / zelf.slots.init, so a Rust-defined dict subtype can inherit this vectorcall but skip its own constructor hooks. That diverges from PyType::call. The same default()+slot_init() pattern in list.rs and bytearray.rs needs the same guard.
🛠️ Conservative fallback fn vectorcall_dict(
zelf_obj: &PyObject,
args: Vec<PyObjectRef>,
nargs: usize,
kwnames: Option<&[PyObjectRef]>,
vm: &VirtualMachine,
) -> PyResult {
let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap();
+ let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames);
+ if !zelf.is(vm.ctx.types.dict_type) {
+ return PyType::call(zelf, func_args, vm);
+ }
let obj = PyDict::default().into_ref_with_type(vm, zelf.to_owned())?;
- let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames);
PyDict::slot_init(obj.clone().into(), func_args, vm)?;
Ok(obj.into())
}Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/dict.rs` around lines 1438 - 1449, The vectorcall_dict fast-path constructs a base dict with PyDict::default().into_ref_with_type(...) and PyDict::slot_init(...) without invoking the type's constructor hooks, so it must detect when the receiver type has custom new/init hooks and fall back to the generic PyType::call path; update vectorcall_dict to check the target type's slots (e.g. inspect zelf.slots.new or zelf.slots.init or whether the type is exactly dict) and if non-default hooks exist (or the type != built-in dict) call PyType::call(zelf_obj, args, vm) (or the equivalent generic constructor path) instead of the fast default+slot_init; apply the same pattern to the analogous fast-paths in list.rs and bytearray.rs to preserve proper subclass constructor semantics.
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/tuple.rs
Comment on lines
+698
to
+709
| fn vectorcall_tuple( | ||
| zelf_obj: &PyObject, | ||
| args: Vec<PyObjectRef>, | ||
| nargs: usize, | ||
| kwnames: Option<&[PyObjectRef]>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap(); | ||
| let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames); | ||
| // Use the type's own slot_new rather than calling PyTuple::slot_new directly, | ||
| // so Rust-level subclasses (e.g. struct sequences) get their custom slot_new called. | ||
| (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args, vm) |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
This still skips subtype slot_init.
Using zelf.slots.new fixes the custom-slot_new case, but the direct return bypasses the post-slot_new init step that PyType::call would run. A Rust-defined tuple subtype with a non-None slots.init will inherit this vectorcall and never see its initializer. The same issue exists in crates/vm/src/builtins/bytes.rs on Lines 89-99.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/tuple.rs` around lines 698 - 709, The vectorcall_tuple implementation uses the type's slot_new but returns immediately, skipping any subtype slot_init; update vectorcall_tuple to mirror PyType::call by invoking the type's slots.new (zelf.slots.new) to create the instance, then if the type has a non-None slots.init (zelf.slots.init.load().is_some()) call that init slot with the newly created object and the original FuncArgs (func_args) and vm before returning the instance; do the same fix in the bytes vectorcall implementation to ensure Rust-defined subtypes receive their post-new initializer.
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/type.rs
Outdated
Comment thread
crates/vm/src/types/slot.rs
Comment on lines
+828
to
+833
| SlotAccessor::TpInit => { | ||
| update_main_slot!(init, init_wrapper, Init); | ||
| if ADD { | ||
| self.slots.vectorcall.store(None); | ||
| } | ||
| } |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify where vectorcall is cleared/restored in slot update flows.
rg -n -C3 'SlotAccessor::TpInit|SlotAccessor::TpNew|slots\.vectorcall\.store' crates/vm/src/types/slot.rs
# Check whether any non-init bootstrapping path recomputes vectorcall for heap types.
rg -n -C3 'vectorcall\.store\(Some|vectorcall\.store\(None|update_slot<false>' crates/vm/src/typesRepository: RustPython/RustPython
Length of output: 2674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full TpInit and TpNew case implementations
sed -n '828,841p' crates/vm/src/types/slot.rs
# Look for inherit_from_mro implementation and usage
rg -n 'fn inherit_from_mro|inherit_from_mro' crates/vm/src/types/slot.rs | head -20
# Check if there are other slots that handle vectorcall restoration
rg -n 'vectorcall.store' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 2019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the SlotAccessor enum and understand all variants
rg -n 'enum SlotAccessor' crates/vm/src/types/slot.rs -A 50
# Check how other slot updates handle the ADD/non-ADD distinction
rg -n 'if ADD' crates/vm/src/types/slot.rs | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for where TpInit handling differs from TpNew
# Check the broader context around TpInit
sed -n '800,900p' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 4610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the update_main_slot macro definition
rg -n 'macro_rules! update_main_slot' crates/vm/src/types/slot.rs -A 20
# Find inherit_from_mro implementation
rg -n 'fn inherit_from_mro' crates/vm/src/types/slot.rs -A 30Repository: RustPython/RustPython
Length of output: 1056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if TpCall also has the same issue
sed -n '820,826p' crates/vm/src/types/slot.rs
# Look at the context around TpInit to understand the slot update pattern
sed -n '700,750p' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 2880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for how non-ADD is handled elsewhere in the function
# to see if there's a pattern for handling deletion
rg -n 'if ADD \{' crates/vm/src/types/slot.rs -B 2 -A 8 | head -100Repository: RustPython/RustPython
Length of output: 5021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find inherit_from_mro implementation
rg -n 'fn inherit_from_mro' crates/vm/src/types/slot.rs -A 50 | head -80Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if vectorcall is recomputed anywhere during slot updates
rg -n 'vectorcall' crates/vm/src/types/slot.rs -B 2 -A 2
# Look for any comments about vectorcall behavior
rg -n -C5 'vectorcall' crates/vm/src/types/slot.rs | grep -A 5 -B 5 'inherit\|restore\|compute'Repository: RustPython/RustPython
Length of output: 2034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a method that recomputes vectorcall for heap types
rg -n 'recompute.*vectorcall|update.*vectorcall' crates/vm/src/types/
# Check the SlotAccessor type to understand its structure
sed -n '650,702p' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search more broadly for inherit_from_mro
rg -n 'inherit_from_mro' crates/vm/src/types/slot.rs | head -5
# Get the implementation by searching in the same file
grep -n 'fn inherit_from_mro' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search in the entire types directory
rg -n 'fn inherit_from_mro' crates/vm/src/types/ -A 30Repository: RustPython/RustPython
Length of output: 2334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at how vectorcall is specifically managed
# Check if there's any place where it's computed or cleared on inheritance
rg -n 'vectorcall' crates/vm/src/ --type rust | grep -i 'inherit\|mro\|parent'Repository: RustPython/RustPython
Length of output: 144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full inherit_from_mro implementation to see all slots it handles
sed -n '414,550p' crates/vm/src/types/slot_defs.rsRepository: RustPython/RustPython
Length of output: 6065
🏁 Script executed:
#!/bin/bash
set -euo pipefixpipfail
# Verify the exact behavior: when TpInit/TpCall/TpNew delete happens (non-ADD),
# does the macro actually expand to inherit_from_mro?
# Let's trace through what happens for TpInit on non-ADD
sed -n '702,728p' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if vectorcall is properly initialized/cleared behavior for __init__ and __new__
# Look for any comments explaining the vectorcall clearing logic
rg -n -B5 -A5 'vectorcall.*None' crates/vm/src/types/slot.rsRepository: RustPython/RustPython
Length of output: 1330
Restore vectorcall inheritance on __init__/__new__ deletion.
When deleting __init__ or __new__ (non-ADD path), inherit_from_mro restores the slot from the MRO but does not restore vectorcall. This differs from TpCall, which properly inherits both. Without restoring vectorcall on deletion, heap types fall back to the slow path even when the inherited slot supports fast-path calls, losing performance optimization that was available in the parent class.
The fix should mirror TpCall's behavior in inherit_from_mro (slot_defs.rs line 456): include vectorcall restoration for TpInit and TpNew cases, not just the primary slots.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/types/slot.rs` around lines 828 - 833, The inherit_from_mro handling for SlotAccessor::TpInit (and TpNew) must also restore the vectorcall slot when the primary slot is deleted: mirror the TpCall logic by copying the ancestor's vectorcall into self.slots.vectorcall in the non-ADD (deletion) path so heap types regain fast-path calls; locate the TpInit/TpNew cases (where update_main_slot! is used for init/init_wrapper/Init) and add the same vectorcall restoration that TpCall performs in inherit_from_mro.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)crates/vm/src/builtins/tuple.rs (1)🤖 Prompt for all review comments with AI agents698-709: ⚠️ Potential issue | 🟠 Major
vectorcall_tuple skips subtype slot_init after slot_new.
This path returns right after slot_new, so subtype init hooks are bypassed. It should mirror PyType::call’s new→init sequence when applicable.
💡 Suggested fix🤖 Prompt for AI Agentsfn vectorcall_tuple( zelf_obj: &PyObject, args: Vec<PyObjectRef>, nargs: usize, kwnames: Option<&[PyObjectRef]>, vm: &VirtualMachine, ) -> PyResult { let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap(); let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames); - // Use the type's own slot_new rather than calling PyTuple::slot_new directly, - // so Rust-level subclasses (e.g. struct sequences) get their custom slot_new called. - (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args, vm) + let obj = (zelf.slots.new.load().unwrap())(zelf.to_owned(), func_args.clone(), vm)?; + if obj.class().fast_issubclass(zelf) { + if let Some(init_method) = obj.class().slots.init.load() { + init_method(obj.clone(), func_args, vm)?; + } + } + Ok(obj) }Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/tuple.rs` around lines 698 - 709, vectorcall_tuple currently calls the type's slot_new and returns immediately, skipping any subclass slot_init; update vectorcall_tuple to mirror PyType::call's new→init sequence: after invoking (zelf.slots.new.load().unwrap()) and obtaining the created object, check whether the type has a non-default slot_init (e.g., via zelf.slots.init.load() or the same mechanism used in PyType::call), and if so call that init slot with the same FuncArgs and vm; propagate any error from slot_init and return the created instance on success, ensuring ownership/borrowing follows the existing pattern (use zelf.to_owned() and the created PyObjectRef as in the surrounding code).
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/stdlib/os.rs`: - Around line 1378-1379: The code that populates the st_atime_ns/st_mtime_ns/st_ctime_ns fields is assigning raw second values (items[i], etc.) directly (see vm.is_none and items usage), but the *_ns contract expects nanoseconds; change the assignment so when the corresponding *_ns slot (e.g., items[i+?] where st_*_ns are filled) is missing you assign the seconds value multiplied by 1_000_000_000 (i.e., seconds * 1_000_000_000) rather than the raw seconds clone; preserve the numeric type used elsewhere and ensure you perform the multiplication before assigning to items[...] for st_atime_ns, st_mtime_ns and st_ctime_ns. --- Duplicate comments: In `@crates/vm/src/builtins/tuple.rs`: - Around line 698-709: vectorcall_tuple currently calls the type's slot_new and returns immediately, skipping any subclass slot_init; update vectorcall_tuple to mirror PyType::call's new→init sequence: after invoking (zelf.slots.new.load().unwrap()) and obtaining the created object, check whether the type has a non-default slot_init (e.g., via zelf.slots.init.load() or the same mechanism used in PyType::call), and if so call that init slot with the same FuncArgs and vm; propagate any error from slot_init and return the created instance on success, ensuring ownership/borrowing follows the existing pattern (use zelf.to_owned() and the created PyObjectRef as in the surrounding code).
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ef159242-c3e7-44ca-9737-7266cebe3ec4
📥 CommitsReviewing files that changed from the base of the PR and between a9ee2e3 and 3c666d6.
📒 Files selected for processing (13)- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/bytearray.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/builtins/float.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/set.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/types/slot.rs
- crates/vm/src/types/slot_defs.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/bytearray.rs
- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/set.rs
- crates/vm/src/builtins/type.rs
Sorry, something went wrong.
Comment thread
crates/vm/src/stdlib/os.rs
Outdated
Comment on lines
+1378
to
+1379
| if vm.is_none(&items[i + 6]) { | ||
| items[i + 6] = items[i].clone(); |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
*_ns fields are populated in seconds, not nanoseconds.
On Line 1379, missing st_atime_ns/st_mtime_ns/st_ctime_ns are set to the raw integer-second values. That breaks the _ns contract (1 second should become 1_000_000_000 ns).
💡 Suggested fix+ let billion = vm.ctx.new_int(1_000_000_000).into();
for i in 7..=9 {
if vm.is_none(&items[i + 3]) {
items[i + 3] = items[i].clone();
}
if vm.is_none(&items[i + 6]) {
- items[i + 6] = items[i].clone();
+ items[i + 6] = vm._mul(&items[i], &billion)?;
}
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if vm.is_none(&items[i + 6]) { | |
| items[i + 6] = items[i].clone(); | |
| let billion = vm.ctx.new_int(1_000_000_000).into(); | |
| for i in 7..=9 { | |
| if vm.is_none(&items[i + 3]) { | |
| items[i + 3] = items[i].clone(); | |
| } | |
| if vm.is_none(&items[i + 6]) { | |
| items[i + 6] = vm._mul(&items[i], &billion)?; | |
| } | |
| } |
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/os.rs` around lines 1378 - 1379, The code that populates the st_atime_ns/st_mtime_ns/st_ctime_ns fields is assigning raw second values (items[i], etc.) directly (see vm.is_none and items usage), but the *_ns contract expects nanoseconds; change the assignment so when the corresponding *_ns slot (e.g., items[i+?] where st_*_ns are filled) is missing you assign the seconds value multiplied by 1_000_000_000 (i.e., seconds * 1_000_000_000) rather than the raw seconds clone; preserve the numeric type used elsewhere and ensure you perform the multiplication before assigning to items[...] for st_atime_ns, st_mtime_ns and st_ctime_ns.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)crates/vm/src/stdlib/os.rs (1)🤖 Prompt for all review comments with AI agentscrates/vm/src/builtins/list.rs (1)1371-1379: ⚠️ Potential issue | 🟠 Major
st_*_ns fallback handling is missing.
Line 1374 leaves nanosecond fields unset when omitted. That can break st_atime_ns/st_mtime_ns/st_ctime_ns consistency for sequence-based construction.
💡 Suggested fix🤖 Prompt for AI Agentsfor i in 7..=9 { if vm.is_none(&items[i + 3]) { items[i + 3] = items[i].clone(); } + if vm.is_none(&items[i + 6]) { + let billion: PyObjectRef = vm.ctx.new_int(1_000_000_000).into(); + items[i + 6] = vm._mul(&items[i], &billion)?; + } }Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/os.rs` around lines 1371 - 1379, The loop that copies integer time fields to float timestamp slots (for i in 7..=9) neglects to set the corresponding nanosecond slots (items[i+6]) when they are omitted; update the loop in the same block to check vm.is_none(&items[i + 6]) and, if missing, set items[i + 6] to a zero-nanoseconds value (e.g., an integer zero) so st_atime_ns/st_mtime_ns/st_ctime_ns are populated consistently alongside the float timestamps and integer seconds.crates/vm/src/builtins/dict.rs (1)771-782: ⚠️ Potential issue | 🟠 Major
This fast path also skips subtype slot_new.
PyList::default().into_ref_with_type(...) has the same hazard as dict: a Rust-defined list subtype can inherit this vectorcall and be allocated as a plain PyList instead of going through its own constructor path. Please guard this fast path on the builtin list slots or fall back to the generic type call path.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/list.rs` around lines 771 - 782, The fast path in vectorcall_list incorrectly constructs a PyList for arbitrary subtypes via PyList::default().into_ref_with_type(...), skipping subtype construction; change vectorcall_list to only take the fast path when the target type is the exact builtin list type or when its new/slot_new is the builtin implementation (i.e., check the type against vm.ctx.types.list_type or compare its slot_new to the builtin slot), and otherwise fall back to the generic type call path so subclass constructors (slot_new) run; keep references to vectorcall_list, PyList::default().into_ref_with_type, PyList::slot_init and slot_new to locate and update the logic.1438-1449: ⚠️ Potential issue | 🟠 Major
This still bypasses subtype slot_new.
PyDict::default().into_ref_with_type(...) never consults the target type’s own constructor slots, so a Rust-defined dict subtype can inherit this vectorcall and skip its allocator/init hooks here. Please fall back to the generic type call path unless the target is still using dict’s builtin new/init slots.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/dict.rs` around lines 1438 - 1449, vectorcall_dict currently creates an instance with PyDict::default().into_ref_with_type(...) which bypasses the target type’s own constructor slots; change it to first detect whether the target type still uses dict’s builtin new/init slots (i.e., compare the target type’s slot_new/slot_init to PyDict’s defaults or check type identity), and only use PyDict::default().into_ref_with_type + PyDict::slot_init when they match; otherwise fall back to the generic type call path (invoke the type’s normal call/constructor path via the VM so overridden alloc/init run) instead of instantiating directly.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/types/slot_defs.rs`: - Around line 456-469: The current inheritance code gates copying a parent type's constructor fast-path behind the presence of an instance call slot (it checks cls.slots.call before taking cls.slots.vectorcall), which prevents subclasses of builtins (e.g., float/list) from inheriting constructor vectorcall; change the find_map closure used to compute inherited_vc to unconditionally return cls.slots.vectorcall.load() (i.e., remove the if cls.slots.call.load().is_some() branch) so typ.slots.vectorcall.store(inherited_vc) can copy constructor vectorcall regardless of the parent's slots.call presence; apply the same removal of the call-slot gating in the corresponding copy/inheritance site that sets vectorcall (the symmetric code that assigns typ.slots.vectorcall). --- Duplicate comments: In `@crates/vm/src/builtins/dict.rs`: - Around line 1438-1449: vectorcall_dict currently creates an instance with PyDict::default().into_ref_with_type(...) which bypasses the target type’s own constructor slots; change it to first detect whether the target type still uses dict’s builtin new/init slots (i.e., compare the target type’s slot_new/slot_init to PyDict’s defaults or check type identity), and only use PyDict::default().into_ref_with_type + PyDict::slot_init when they match; otherwise fall back to the generic type call path (invoke the type’s normal call/constructor path via the VM so overridden alloc/init run) instead of instantiating directly. In `@crates/vm/src/builtins/list.rs`: - Around line 771-782: The fast path in vectorcall_list incorrectly constructs a PyList for arbitrary subtypes via PyList::default().into_ref_with_type(...), skipping subtype construction; change vectorcall_list to only take the fast path when the target type is the exact builtin list type or when its new/slot_new is the builtin implementation (i.e., check the type against vm.ctx.types.list_type or compare its slot_new to the builtin slot), and otherwise fall back to the generic type call path so subclass constructors (slot_new) run; keep references to vectorcall_list, PyList::default().into_ref_with_type, PyList::slot_init and slot_new to locate and update the logic. In `@crates/vm/src/stdlib/os.rs`: - Around line 1371-1379: The loop that copies integer time fields to float timestamp slots (for i in 7..=9) neglects to set the corresponding nanosecond slots (items[i+6]) when they are omitted; update the loop in the same block to check vm.is_none(&items[i + 6]) and, if missing, set items[i + 6] to a zero-nanoseconds value (e.g., an integer zero) so st_atime_ns/st_mtime_ns/st_ctime_ns are populated consistently alongside the float timestamps and integer seconds.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1c7adec1-9b97-4816-a120-0a8b7fb9c5d7
📥 CommitsReviewing files that changed from the base of the PR and between 3c666d6 and 2ef3473.
📒 Files selected for processing (12)- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/builtins/float.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/set.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/types/slot.rs
- crates/vm/src/types/slot_defs.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/types/slot.rs
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/set.rs
Comment on lines
+1424
to
+1435
| fn vectorcall_set( | ||
| zelf_obj: &PyObject, | ||
| args: Vec<PyObjectRef>, | ||
| nargs: usize, | ||
| kwnames: Option<&[PyObjectRef]>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap(); | ||
| let obj = PySet::default().into_ref_with_type(vm, zelf.to_owned())?; | ||
| let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames); | ||
| PySet::slot_init(obj.clone().into(), func_args, vm)?; | ||
| Ok(obj.into()) |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
vectorcall_set bypasses subtype construction.
Unlike vectorcall_frozenset just below, this always allocates PySet directly and never consults zelf.slots.new. A Rust-defined set subtype can therefore inherit the vectorcall slot but skip its own constructor path. Please guard this fast path on the builtin set slots or fall back to the generic type call path.
Conservative fallback use crate::{
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject,
atomic_func,
class::PyClassImpl,
common::{ascii, hash::PyHash, lock::PyMutex, rc::PyRc, wtf8::Wtf8Buf},
convert::ToPyResult,
dict_inner::{self, DictSize},
function::{ArgIterable, FuncArgs, OptionalArg, PosArgs, PyArithmeticValue, PyComparisonValue},
protocol::{PyIterReturn, PyNumberMethods, PySequenceMethods},
recursion::ReprGuard,
types::AsNumber,
types::{
- AsSequence, Comparable, Constructor, DefaultConstructor, Hashable, Initializer, IterNext,
- Iterable, PyComparisonOp, Representable, SelfIter,
+ AsSequence, Callable, Comparable, Constructor, DefaultConstructor, Hashable, Initializer,
+ IterNext, Iterable, PyComparisonOp, Representable, SelfIter,
},
utils::collection_repr,
vm::VirtualMachine,
};
@@
fn vectorcall_set(
zelf_obj: &PyObject,
args: Vec<PyObjectRef>,
nargs: usize,
kwnames: Option<&[PyObjectRef]>,
vm: &VirtualMachine,
) -> PyResult {
let zelf: &Py<PyType> = zelf_obj.downcast_ref().unwrap();
+ let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames);
+ if !zelf.is(vm.ctx.types.set_type) {
+ return PyType::call(zelf, func_args, vm);
+ }
let obj = PySet::default().into_ref_with_type(vm, zelf.to_owned())?;
- let func_args = FuncArgs::from_vectorcall_owned(args, nargs, kwnames);
PySet::slot_init(obj.clone().into(), func_args, vm)?;
Ok(obj.into())
}
Sorry, something went wrong.
Comment thread
crates/vm/src/types/slot_defs.rs
Add vectorcall fast paths for dict, list, set, int, float, str, bool, tuple, frozenset. Clear vectorcall when __init__/__new__ is overridden in Python. Prevent constructor vectorcall inheritance to heap subclasses. Fix stat_result to use struct_sequence_new with reference-copy for hidden time fields.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ℹ️ Review info ⚙️ Run configuration
crates/vm/src/types/slot_defs.rs (1)🤖 Prompt for all review comments with AI agents462-468: ⚠️ Potential issue | 🟠 Major
Constructor vectorcall inheritance is blocked for non-callable builtins.
Line 462 and Line 590 gate slots.vectorcall inheritance on slots.call. That prevents subclasses of constructor-only builtins (dict, list, int, etc.) from inheriting constructor vectorcall and forces fallback to generic construction (crates/vm/src/builtins/type.rs, Line 2686).
Suggested fixSelf::TpCall => { inherit_main!(call); - let inherited_vc = mro.iter().find_map(|cls| { - if cls.slots.call.load().is_some() { - cls.slots.vectorcall.load() - } else { - None - } - }); + let inherited_vc = mro.iter().find_map(|cls| cls.slots.vectorcall.load()); typ.slots.vectorcall.store(inherited_vc); }Self::TpCall => { copy_main!(call); - if typ.slots.vectorcall.load().is_none() - && base.slots.call.load().is_some() - && let Some(base_val) = base.slots.vectorcall.load() + if typ.slots.vectorcall.load().is_none() + && let Some(base_val) = base.slots.vectorcall.load() { typ.slots.vectorcall.store(Some(base_val)); } }Also applies to: 590-594
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/types/slot_defs.rs` around lines 462 - 468, The code currently prevents inheriting slots.vectorcall when slots.call is absent (see the inherited_vc computation using mro.iter().find_map), which blocks constructor-only builtins from inheriting vectorcall; change the predicate to check only for the presence of cls.slots.vectorcall (not cls.slots.call) so vectorcall is inherited independently of call. Apply the same fix to the other similar block that conditions vectorcall on call (the second mro.iter().find_map instance around slots.vectorcall) so subclasses of constructor-only builtins can inherit constructor vectorcall.
Verify each finding against the current code and only fix it if needed. Duplicate comments: In `@crates/vm/src/types/slot_defs.rs`: - Around line 462-468: The code currently prevents inheriting slots.vectorcall when slots.call is absent (see the inherited_vc computation using mro.iter().find_map), which blocks constructor-only builtins from inheriting vectorcall; change the predicate to check only for the presence of cls.slots.vectorcall (not cls.slots.call) so vectorcall is inherited independently of call. Apply the same fix to the other similar block that conditions vectorcall on call (the second mro.iter().find_map instance around slots.vectorcall) so subclasses of constructor-only builtins can inherit constructor vectorcall.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 348b0c7b-19d8-453b-91ae-a97e06e7c799
📥 CommitsReviewing files that changed from the base of the PR and between 2ef3473 and f76e0d1.
📒 Files selected for processing (12)- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/builtins/float.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/set.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/tuple.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/types/slot.rs
- crates/vm/src/types/slot_defs.rs
- crates/vm/src/builtins/bool.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/types/slot.rs
- crates/vm/src/builtins/tuple.rs
Sorry, something went wrong.
youknowone
force-pushed
the
vectorcall
branch
2 times, most recently
from
ff165a2 to
f76e0d1
Compare
March 17, 2026 14:16
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Mar 19, 2026
Add vectorcall fast paths for dict, list, set, int, float, str, bool, tuple, frozenset. Clear vectorcall when __init__/__new__ is overridden in Python. Prevent constructor vectorcall inheritance to heap subclasses. Fix stat_result to use struct_sequence_new with reference-copy for hidden time fields.
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
Add vectorcall fast paths for dict, list, set, int, float, str, bool, tuple, frozenset. Clear vectorcall when __init__/__new__ is overridden in Python. Prevent constructor vectorcall inheritance to heap subclasses. Fix stat_result to use struct_sequence_new with reference-copy for hidden time fields.
Copilot AI
pushed a commit
that referenced
this pull request
Mar 25, 2026
Add vectorcall fast paths for dict, list, set, int, float, str, bool, tuple, frozenset. Clear vectorcall when __init__/__new__ is overridden in Python. Prevent constructor vectorcall inheritance to heap subclasses. Fix stat_result to use struct_sequence_new with reference-copy for hidden time fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #7362
Add vectorcall dispatch in vectorcall_type to check each type's own slots.vectorcall before falling back to PyType::call. Clear vectorcall when init or new is overridden in update_slot to ensure subclass correctness.
Implement per-type vectorcall for 12 builtin types:
Summary by CodeRabbit