Skip to content

Take a borrow in invoke_exception instead of an owned type - #8327

Merged
youknowone merged 2 commits into
RustPython:mainfrom
kangdora:invoke-exception-borrow
Jul 20, 2026
Merged

Take a borrow in invoke_exception instead of an owned type#8327
youknowone merged 2 commits into
RustPython:mainfrom
kangdora:invoke-exception-borrow

Conversation

@kangdora

@kangdora kangdora commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

VirtualMachine::invoke_exception took its class argument as an owned PyTypeRef, but it never takes ownership β€” it only borrows cls for PyType::call (which itself is defined as fn call(zelf: &Py<Self>, ...)) and for Display in the error message. cls is never moved or stored.

Because the signature demanded ownership, callers holding only a borrow had to .to_owned() β€” an atomic refcount bump and later decrement β€” purely to satisfy the type, even though the reference was dropped immediately after the call. This changes the parameter to &Py<PyType>, the exact borrowed form (PyTypeRef = PyRef<PyType> derefs to Py<PyType>), and drops those .to_owned() calls.

Call sites updated:

file before
stdlib/_io.rs (3Γ—) vm.ctx.exceptions.blocking_io_error.to_owned()
stdlib/sys.rs vm.ctx.exceptions.system_exit.to_owned()
stdlib/builtins.rs vm.ctx.exceptions.system_exit.to_owned()
stdlib/_thread.rs vm.ctx.exceptions.system_exit.to_owned()
vm/mod.rs vm.ctx.exceptions.system_exit.to_owned()
vm/vm_new.rs vm.ctx.exceptions.stop_iteration.to_owned()
exception_group.rs vm.ctx.exceptions.base_exception_group.to_owned()
exceptions.rs typ.to_owned()
capi/pyerrors.rs exc_type.to_owned()

ExceptionCtor::instantiate / instantiate_value own their cls via the self match, so they just pass &cls β€” no clone either way.

No behavior change; this is an API/ergonomics cleanup that removes unnecessary refcount churn and makes the helper consistent with the PyType::call it wraps.

Test plan

  • test_exceptions, test_sys, test_io (667 tests, covering the BlockingIOError sites), extra_tests/snippets/builtin_exceptions.py: no regressions (the single test_exceptions failure, test_badisinstance, is pre-existing on main).
  • Exercised each touched path manually: sys.exit / exit() / _thread.exit, raise ValueError and raise ValueError(...) via ExceptionCtor, ExceptionGroup, generator StopIteration, OSError from a failed open, and the genuine Err path of invoke_exception (a BaseException subclass whose __new__ returns a non-exception).
  • cargo fmt --check / cargo clippy: clean.

Assisted-by: Claude Code:claude-fable-5

Summary by CodeRabbit

  • Refactor
    • Streamlined internal exception creation across the runtime by avoiding unnecessary exception-type copies.
    • Preserved existing behavior for SystemExit, BlockingIOError, StopIteration, COM errors, and other exception paths.
    • Improved exception handling efficiency without changing public functionality or error messages.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. πŸŽ‰

ℹ️ Recent review info
βš™οΈ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 20542d31-6ab2-4a97-bb5b-dff6c39c506f

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 3290f28 and 958ae0e.

πŸ“’ Files selected for processing (10)
  • crates/capi/src/pyerrors.rs
  • crates/vm/src/exception_group.rs
  • crates/vm/src/exceptions.rs
  • crates/vm/src/stdlib/_ctypes/function.rs
  • crates/vm/src/stdlib/_io.rs
  • crates/vm/src/stdlib/_thread.rs
  • crates/vm/src/stdlib/builtins.rs
  • crates/vm/src/stdlib/sys.rs
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/vm/vm_new.rs

πŸ“ Walkthrough

Walkthrough

invoke_exception now accepts borrowed exception types. Its internal constructors and exception-raising call sites were updated to pass references directly instead of cloning exception type objects.

Changes

Exception type borrowing

Layer / File(s) Summary
Update exception construction API
crates/vm/src/exceptions.rs, crates/capi/src/pyerrors.rs
VirtualMachine::invoke_exception now accepts &Py<PyType>, with internal constructors and PyErr_SetString updated accordingly.
Migrate exception raise call sites
crates/vm/src/exception_group.rs, crates/vm/src/stdlib/*, crates/vm/src/vm/*
SystemExit, BlockingIOError, COMError, BaseExceptionGroup, and StopIteration paths pass exception types without .to_owned().

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

Suggested labels: z-ca-2026

Suggested reviewers: shaharnaveh, youknowone

πŸš₯ Pre-merge checks | βœ… 5
βœ… Passed checks (5 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title clearly summarizes the main API change: invoke_exception now borrows the exception type instead of taking ownership.
Docstring Coverage βœ… Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check βœ… Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check βœ… Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests

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.

❀️ Share

Comment @coderabbitai help to get the list of available commands.

@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 20, 2026

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ‘

@youknowone
youknowone merged commit eb56114 into RustPython:main Jul 20, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Take &Py<PyType> instead of PyTypeRef in vm.invoke_exception()

2 participants