From 2daddb4734a0404b263807d859d869512add0e60 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 15 Sep 2023 15:04:03 +0200 Subject: [PATCH] unify 3.12 and pre-3.12 exception handling pathways --- newsfragments/3455.changed.md | 1 + pytests/src/pyclasses.rs | 4 ++++ pytests/tests/test_pyclasses.py | 26 +++++++++++++++++++++++++- src/err/err_state.rs | 18 ++++++------------ 4 files changed, 36 insertions(+), 13 deletions(-) create mode 100644 newsfragments/3455.changed.md diff --git a/newsfragments/3455.changed.md b/newsfragments/3455.changed.md new file mode 100644 index 00000000000..53944f9a05f --- /dev/null +++ b/newsfragments/3455.changed.md @@ -0,0 +1 @@ +`Err` returned from `#[pyfunction]` will now have a non-None `__context__` if called from inside a `catch` block. diff --git a/pytests/src/pyclasses.rs b/pytests/src/pyclasses.rs index 48fa628b0c7..46c8523c2dd 100644 --- a/pytests/src/pyclasses.rs +++ b/pytests/src/pyclasses.rs @@ -58,10 +58,14 @@ impl AssertingBaseClass { } } +#[pyclass] +struct ClassWithoutConstructor; + #[pymodule] pub fn pyclasses(_py: Python<'_>, m: &PyModule) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; Ok(()) } diff --git a/pytests/tests/test_pyclasses.py b/pytests/tests/test_pyclasses.py index 4a45b413669..5482862811a 100644 --- a/pytests/tests/test_pyclasses.py +++ b/pytests/tests/test_pyclasses.py @@ -1,3 +1,5 @@ +from typing import Type + import pytest from pyo3_pytests import pyclasses @@ -32,7 +34,29 @@ class AssertingSubClass(pyclasses.AssertingBaseClass): def test_new_classmethod(): - # The `AssertingBaseClass` constructor errors if it is not passed the relevant subclass. + # The `AssertingBaseClass` constructor errors if it is not passed the + # relevant subclass. _ = AssertingSubClass(expected_type=AssertingSubClass) with pytest.raises(ValueError): _ = AssertingSubClass(expected_type=str) + + +class ClassWithoutConstructorPy: + def __new__(cls): + raise TypeError("No constructor defined") + + +@pytest.mark.parametrize( + "cls", [pyclasses.ClassWithoutConstructor, ClassWithoutConstructorPy] +) +def test_no_constructor_defined_propagates_cause(cls: Type): + original_error = ValueError("Original message") + with pytest.raises(Exception) as exc_info: + try: + raise original_error + except Exception: + cls() # should raise TypeError("No constructor defined") + + assert exc_info.type is TypeError + assert exc_info.value.args == ("No constructor defined",) + assert exc_info.value.__context__ is original_error diff --git a/src/err/err_state.rs b/src/err/err_state.rs index 8e5dcf176c4..b9bda9b7013 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -1,5 +1,3 @@ -#[cfg(not(Py_3_12))] -use crate::types::PyString; use crate::{ exceptions::{PyBaseException, PyTypeError}, ffi, @@ -195,17 +193,14 @@ fn lazy_into_normalized_ffi_tuple( py: Python<'_>, lazy: Box, ) -> (*mut ffi::PyObject, *mut ffi::PyObject, *mut ffi::PyObject) { - let PyErrStateLazyFnOutput { ptype, pvalue } = lazy(py); - let (mut ptype, mut pvalue) = if unsafe { ffi::PyExceptionClass_Check(ptype.as_ptr()) } == 0 { - ( - PyTypeError::type_object_raw(py).cast(), - PyString::new(py, "exceptions must derive from BaseException").into_ptr(), - ) - } else { - (ptype.into_ptr(), pvalue.into_ptr()) - }; + // To be consistent with 3.12 logic, go via raise_lazy, but also then normalize + // the resulting exception + raise_lazy(py, lazy); + let mut ptype = std::ptr::null_mut(); + let mut pvalue = std::ptr::null_mut(); let mut ptraceback = std::ptr::null_mut(); unsafe { + ffi::PyErr_Fetch(&mut ptype, &mut pvalue, &mut ptraceback); ffi::PyErr_NormalizeException(&mut ptype, &mut pvalue, &mut ptraceback); } (ptype, pvalue, ptraceback) @@ -218,7 +213,6 @@ fn lazy_into_normalized_ffi_tuple( /// /// This would require either moving some logic from C to Rust, or requesting a new /// API in CPython. -#[cfg(Py_3_12)] fn raise_lazy(py: Python<'_>, lazy: Box) { let PyErrStateLazyFnOutput { ptype, pvalue } = lazy(py); unsafe {