Repository navigation
Take &Py<PyType> instead of PyTypeRef in vm.invoke_exception() #8320
Description
Summary
VirtualMachine::invoke_exception currently takes its class argument by owned value:
// crates/vm/src/exceptions.rs
pub fn invoke_exception(
&self,
cls: PyTypeRef, // PyRef<PyType>
args: Vec<PyObjectRef>,
) -> PyResult<PyBaseExceptionRef> {
let res = PyType::call(&cls, args.into_args(self), self)?;
res.downcast::<PyBaseException>().map_err(|obj| {
self.new_type_error(format!(
"calling {} should have returned an instance of BaseException, not {}",
cls,
obj.class()
))
})
}It should take a borrow instead:
pub fn invoke_exception(
&self,
cls: &Py<PyType>,
args: Vec<PyObjectRef>,
) -> PyResult<PyBaseExceptionRef> { ... }Why the change is justified
1. invoke_exception never takes ownership of cls.
The body only ever borrows it:
PyType::call(&cls, ...)—PyType::callis defined asfn call(zelf: &Py<Self>, ...), i.e. it already takes a borrow.clsin thenew_type_errorformat string is used viaDisplay, by reference.
There is no point where cls is moved, stored, or otherwise consumed. The owned PyTypeRef parameter is therefore strictly stronger than what the function needs. PyTypeRef (= PyRef<PyType>) derefs to Py<PyType>, so &Py<PyType> is the exact borrowed form.
2. The owned parameter forces needless refcount churn at call sites.
Because the signature demands ownership, most callers only holding a borrow must .to_owned() — an atomic refcount increment (and later decrement) — purely to satisfy the type, even though the reference is dropped immediately after the call returns:
| Call site | Current argument |
|---|---|
crates/vm/src/stdlib/_io.rs:945 |
vm.ctx.exceptions.blocking_io_error.to_owned() |
crates/vm/src/stdlib/_io.rs:1156 |
vm.ctx.exceptions.blocking_io_error.to_owned() |
crates/vm/src/stdlib/_io.rs:1202 |
vm.ctx.exceptions.blocking_io_error.to_owned() |
crates/vm/src/stdlib/sys.rs:779 |
vm.ctx.exceptions.system_exit.to_owned() |
crates/vm/src/exception_group.rs:74 |
vm.ctx.exceptions.base_exception_group.to_owned() |
crates/vm/src/exceptions.rs:2010 |
typ.to_owned() |
crates/capi/src/pyerrors.rs:152 |
exc_type.to_owned() |
With &Py<PyType> every one of these drops the .to_owned() and passes the borrow directly.
3. It matches the idiomatic borrow used elsewhere.
&Py<T> is the conventional "borrowed typed object" throughout the VM, and specifically the method invoke_exception delegates to — PyType::call(zelf: &Py<Self>, ...) — already takes exactly that. The change makes the helper consistent with what it wraps.
4. Callers that legitimately own a PyTypeRef are unaffected in cost.
ExceptionCtor::instantiate / instantiate_value (crates/vm/src/exceptions.rs:447 and :475) own their cls via the self match; they just pass &cls instead of moving it — a trivial edit and no clone either way.
Proposed change
- Change the parameter to
cls: &Py<PyType>. - Update the ~9 call sites: drop
.to_owned()where a borrow is already in hand, and pass&clsfrom theExceptionCtorsites.
No behavior change; this is a pure API/ergonomics cleanup that removes several unnecessary refcount bumps.
Activity
kangdora commentedon Jul 19, 2026
take