Skip to content

port PyErr::from_type to Bound API #3885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
davidhewitt merged 1 commit into from
Feb 23, 2024
Merged

port PyErr::from_type to Bound API #3885

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions src/err/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,21 @@ impl PyErr {
})))
}

/// Deprecated form of [`PyErr::from_type_bound`]
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyErr::from_type` will be replaced by `PyErr::from_type_bound` in a future PyO3 version"
)
)]
pub fn from_type<A>(ty: &PyType, args: A) -> PyErr
where
A: PyErrArguments + Send + Sync + 'static,
{
PyErr::from_state(PyErrState::lazy(ty.into(), args))
}

/// Constructs a new PyErr from the given Python type and arguments.
///
/// `ty` is the exception type; usually one of the standard exceptions
Expand All @@ -192,11 +207,11 @@ impl PyErr {
/// If `ty` does not inherit from `BaseException`, then a `TypeError` will be returned.
///
/// If calling `ty` with `args` raises an exception, that exception will be returned.
pub fn from_type<A>(ty: &PyType, args: A) -> PyErr
pub fn from_type_bound<A>(ty: Bound<'_, PyType>, args: A) -> PyErr
Copy link
Contributor Author

@Icxolu Icxolu Feb 22, 2024

Choose a reason for hiding this comment

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

Arguably this could also just take a Py<PyType>, but then we would need a different name I guess...

Copy link
Member

@davidhewitt davidhewitt Feb 22, 2024

Choose a reason for hiding this comment

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

Ugh, yes. How about new_with_type?

I'm also kinda ok with leaving this as-is; it's a bit easier to get a Bound<PyType> than a Py<PyType> (e.g. with PyAnyMethods::get_type(), or just .downcast_into())

Copy link
Contributor Author

@Icxolu Icxolu Feb 22, 2024

Choose a reason for hiding this comment

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

What do you prefer? I think its fine either way.

Copy link
Member

@davidhewitt davidhewitt Feb 22, 2024

Choose a reason for hiding this comment

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

For simplicity of migration let's just keep this as is, given there's no obvious answer.

where
A: PyErrArguments + Send + Sync + 'static,
{
PyErr::from_state(PyErrState::lazy(ty.into(), args))
PyErr::from_state(PyErrState::lazy(ty.unbind().into_any(), args))
}

/// Deprecated form of [`PyErr::from_value_bound`].
Expand Down Expand Up @@ -1231,10 +1246,8 @@ mod tests {
assert!(!err.matches(py, PyTypeError::type_object_bound(py)));

// String is not a valid exception class, so we should get a TypeError
let err: PyErr = PyErr::from_type(
crate::types::PyString::type_object_bound(py).as_gil_ref(),
"foo",
);
let err: PyErr =
PyErr::from_type_bound(crate::types::PyString::type_object_bound(py), "foo");
assert!(err.matches(py, PyTypeError::type_object_bound(py)));
})
}
Expand Down