Skip to content

Add new_payload_exception helper for constructing built-in payload exceptions - #8403

Draft
kangdora wants to merge 1 commit into
RustPython:mainfrom
kangdora:generic-payload-exception
Draft

Add new_payload_exception helper for constructing built-in payload exceptions#8403
kangdora wants to merge 1 commit into
RustPython:mainfrom
kangdora:generic-payload-exception

Conversation

@kangdora

Copy link
Copy Markdown
Contributor

Follow-up to the discussion in #8348.

Built-in exceptions that carry a payload (a #[repr(C)] struct with extra
fields) are currently constructed either through PyType::call
(via invoke_exception) or through a bespoke per-type builder like
OSErrorBuilder. This adds a generic helper for the internal builders that
know their exact type at compile time, so they can construct directly
(py_new + slot_init) without going through PyType::call:

pub fn new_payload_exception<T>(&self, cls: PyTypeRef, args: FuncArgs) -> PyResult<PyRef<T>>
where
    T: Constructor<Args = FuncArgs> + Initializer,
{
    let payload = T::py_new(&cls, args.clone(), self)?;
    // cls not matching T structurally is a caller bug, not a runtime error
    let exc = payload
        .into_ref_with_type_lazy_dict(self, cls)
        .expect("new_payload_exception: cls is not a matching subtype of T");
    T::slot_init(exc.as_object().to_owned(), args, self)?;
    Ok(exc)
}

new_exception stays as the thin, restricted fast path for payload-free
types, and invoke_exception still handles the runtime-typed / user-subclass
path (raise <expr>, C-API) — a helper parameterized on a compile-time T
can't respect a user subclass's overridden __new__/__init__.

Error handling: only into_ref_with_type_lazy_dict is .expected — it
fails when cls doesn't structurally match T, which is a caller mistake that
user input can't cause. py_new and slot_init propagate with ?, so a
legitimately-rejected argument (e.g. __init__ refusing a bad value) surfaces
as a normal Python exception.

Routes new_stop_iteration and OSErrorBuilder through the helper as the
first two call sites — the simplest and most complex payload exceptions
respectively. Behaves identically to before and simplifies OSErrorBuilder.

Open questions

Opening as a draft mainly to get feedback on a couple of points:

  • Owned upcast. Call sites like new_stop_iteration return
    PyBaseExceptionRef, so the helper's PyRef<T> needs an owned upcast. I used
    .upcast(), which goes through a runtime downcast. The zero-cost conversions
    (upcast_ref/to_base) only yield &Py<_>, and into_base downcasts too, so
    I don't see a zero-cost owned conversion here — is the downcast fine to keep?
  • Trait bound. Constructor<Args = FuncArgs> + Initializer doesn't actually
    constrain T to an exception type. Not sure whether that's worth tightening.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 04b13225-11eb-4fa1-91f7-29f23e6fee1e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ 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 28, 2026
@youknowone

Copy link
Copy Markdown
Member

@kangdora do you have any concern to work more on this patch?

@kangdora

kangdora commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, one concern. Since invoke_exception is the shared construction path for every exception, this change touches a lot of call sites at once, so I want to be careful about scope. Sorry this has been sitting quiet, I haven't pushed more on it yet.

Honestly this is a little beyond my current level, so instead of changing everything at once I've been going through the call sites to figure out which ones are actually safe to touch first. Where I've landed so far:

  • Safe to migrate (payload type known at compile time): the SystemExit and BlockingIOError sites. These aren't hot paths, so the value is consistency rather than perf.
  • Has to stay on invoke_exception (type only known at runtime): the ExceptionCtor path, reduce, the C-API entry, and ctypes' COMError. These need to respect user __new__/__init__ overrides, which requires the PyType::call slot dispatch that new_payload_exception deliberately skips.
    There are also a few edge cases that might be migratable too, but I haven't fully convinced myself yet, so I'm leaving them for now.

So rather than limiting it upfront, I'd like to keep working through these gradually as I understand the area better. If I've misjudged any of these boundaries, I'd really appreciate a correction.

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.

2 participants