change PyModule::add_class to return an error if class creation fails #2947

davidhewitt · 2023-02-12T19:24:01Z

Related to #2942

At the moment there are panics deep in the #[pyclass] machinery when initialising the type fails. This PR adjusts a number of these functions to return PyResult instead, so that we can handle the error more appropriately further down the pipeline.

For example, take the following snippet:

#[pyclass(extends = PyBool)]
struct ExtendsBool;

#[pymodule]
fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<ExtendsBool>()?;
    Ok(())
}

Currently, importing this module will fail with a panic:

TypeError: type 'bool' is not an acceptable base type
thread '<unnamed>' panicked at 'An error occurred while initializing class ExtendsBool', /Users/david/Dev/pyo3/src/pyclass.rs:412:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module>
    from .pyo3_scratch import *
pyo3_runtime.PanicException: An error occurred while initializing class ExtendsBool

After this PR, this import still fails, but with a slightly cleaner, more Pythonic error:

TypeError: type 'bool' is not an acceptable base type

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module>
    from .pyo3_scratch import *
RuntimeError: An error occurred while initializing class ExtendsBool

src/impl_/pyclass/static_type_object.rs

pyo3-macros-backend/src/pyclass.rs

tests/test_class_attributes.rs

davidhewitt · 2023-02-14T08:21:25Z

On reflection I've renamed StaticTypeObject to LazyTypeObject (was LazyStaticType before this PR). It's got nothing to do with "static" any more now that we use heap types, and the &'static reference is encoded elsewhere in the PyClassImpl implementation (but this would function perfectly fine without being static).

I've also pushed a follow-up commit which switches this to store Py<PyType> instead of *mut ffi::PyTypeObject to track ownership properly, and adjusts the internals to make use of this and &PyType so there's less unsafe.

src/impl_/pyclass/lazy_type_object.rs

adamreichold

I think this looks much better with the follow-up commit! Just one question where I am not sure about the performance characteristics.

davidhewitt · 2023-02-16T21:37:57Z

Thanks! I've adjusted the comment around the code you've questioned, replied, and will proceed to merge.

If we think there's follow-up work I'll open a new PR.

davidhewitt · 2023-02-16T21:38:08Z

bors r=adamreichold

2947: change PyModule::add_class to return an error if class creation fails r=adamreichold a=davidhewitt Related to #2942 At the moment there are panics deep in the `#[pyclass]` machinery when initialising the type fails. This PR adjusts a number of these functions to return `PyResult` instead, so that we can handle the error more appropriately further down the pipeline. For example, take the following snippet: ```rust #[pyclass(extends = PyBool)] struct ExtendsBool; #[pymodule] fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> { m.add_class::<ExtendsBool>()?; Ok(()) } ``` Currently, importing this module will fail with a panic: ``` TypeError: type 'bool' is not an acceptable base type thread '<unnamed>' panicked at 'An error occurred while initializing class ExtendsBool', /Users/david/Dev/pyo3/src/pyclass.rs:412:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * pyo3_runtime.PanicException: An error occurred while initializing class ExtendsBool ``` After this PR, this import still fails, but with a slightly cleaner, more Pythonic error: ``` TypeError: type 'bool' is not an acceptable base type The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * RuntimeError: An error occurred while initializing class ExtendsBool ``` Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

bors · 2023-02-16T22:26:21Z

Build failed:

conclusion

davidhewitt · 2023-02-18T09:06:57Z

bors retry

2947: change PyModule::add_class to return an error if class creation fails r=adamreichold a=davidhewitt Related to #2942 At the moment there are panics deep in the `#[pyclass]` machinery when initialising the type fails. This PR adjusts a number of these functions to return `PyResult` instead, so that we can handle the error more appropriately further down the pipeline. For example, take the following snippet: ```rust #[pyclass(extends = PyBool)] struct ExtendsBool; #[pymodule] fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> { m.add_class::<ExtendsBool>()?; Ok(()) } ``` Currently, importing this module will fail with a panic: ``` TypeError: type 'bool' is not an acceptable base type thread '<unnamed>' panicked at 'An error occurred while initializing class ExtendsBool', /Users/david/Dev/pyo3/src/pyclass.rs:412:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * pyo3_runtime.PanicException: An error occurred while initializing class ExtendsBool ``` After this PR, this import still fails, but with a slightly cleaner, more Pythonic error: ``` TypeError: type 'bool' is not an acceptable base type The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * RuntimeError: An error occurred while initializing class ExtendsBool ``` Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

bors · 2023-02-18T09:56:53Z

Build failed:

conclusion

tests/test_class_attributes.rs

adamreichold · 2023-02-18T16:01:42Z

bors r+

2947: change PyModule::add_class to return an error if class creation fails r=adamreichold a=davidhewitt Related to #2942 At the moment there are panics deep in the `#[pyclass]` machinery when initialising the type fails. This PR adjusts a number of these functions to return `PyResult` instead, so that we can handle the error more appropriately further down the pipeline. For example, take the following snippet: ```rust #[pyclass(extends = PyBool)] struct ExtendsBool; #[pymodule] fn pyo3_scratch(_py: Python, m: &PyModule) -> PyResult<()> { m.add_class::<ExtendsBool>()?; Ok(()) } ``` Currently, importing this module will fail with a panic: ``` TypeError: type 'bool' is not an acceptable base type thread '<unnamed>' panicked at 'An error occurred while initializing class ExtendsBool', /Users/david/Dev/pyo3/src/pyclass.rs:412:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * pyo3_runtime.PanicException: An error occurred while initializing class ExtendsBool ``` After this PR, this import still fails, but with a slightly cleaner, more Pythonic error: ``` TypeError: type 'bool' is not an acceptable base type The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> File "/Users/david/.virtualenvs/pyo3/lib/python3.10/site-packages/pyo3_scratch/__init__.py", line 1, in <module> from .pyo3_scratch import * RuntimeError: An error occurred while initializing class ExtendsBool ``` Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Co-authored-by: Adam Reichold <adam.reichold@t-online.de>

…olchains.

bors · 2023-02-18T16:11:48Z

Canceled.

adamreichold · 2023-02-18T16:12:01Z

bors retry

bors · 2023-02-18T17:01:27Z

Build succeeded:

conclusion