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