Panic when calling del typ.attr on a [setter] defined attribute

[indygreg]

Using the latest code in main / 0.14.2, it is possible to trigger a panic from Python by calling del on a [setter] defined attribute on a Rust type. This can be reproduced with the following patch to the official test harness:

diff --git a/tests/test_getter_setter.rs b/tests/test_getter_setter.rs
index 53e99803b..2c2cd817d 100644
--- a/tests/test_getter_setter.rs
+++ b/tests/test_getter_setter.rs
@@ -59,6 +59,8 @@ fn class_with_properties() {
     py_run!(py, inst, "assert inst.get_num() == inst.unwrapped == 42");
     py_run!(py, inst, "assert inst.data_list == [42]");

+    py_run!(py, inst, "del inst.DATA");
+
     let d = [("C", py.get_type::<ClassWithProperties>())].into_py_dict(py);
     py_assert!(py, *d, "C.DATA.__doc__ == 'a getter for data'");
 }

Here's context from the crash:

thread 'class_with_properties' panicked at 'Python API call failed', src/err/mod.rs:548:5
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/std/src/panicking.rs:541:12
   1: pyo3::err::panic_after_error
             at ./src/err/mod.rs:548:5
   2: pyo3::conversion::FromPyPointer::from_borrowed_ptr_or_panic::{{closure}}
             at ./src/conversion.rs:506:67
   3: core::option::Option<T>::unwrap_or_else
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/option.rs:429:21
   4: pyo3::conversion::FromPyPointer::from_borrowed_ptr_or_panic
             at ./src/conversion.rs:506:9
   5: pyo3::conversion::FromPyPointer::from_borrowed_ptr
             at ./src/conversion.rs:514:9
   6: pyo3::python::Python::from_borrowed_ptr
             at ./src/python.rs:555:9
   7: test_getter_setter::<impl pyo3::class::impl_::PyMethods<test_getter_setter::ClassWithProperties> for pyo3::class::impl_::PyClassImplCollector<test_getter_setter::ClassWithProperties>>::py_methods::METHODS::__wrap::{{closure}}
             at ./tests/test_getter_setter.rs:12:1
   8: pyo3::callback::handle_panic::{{closure}}
             at ./src/callback.rs:247:9
   9: std::panicking::try::do_call
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/std/src/panicking.rs:401:40
  10: __rust_try
  11: std::panicking::try
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/std/src/panicking.rs:365:19
  12: std::panic::catch_unwind
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/std/src/panic.rs:434:14
  13: pyo3::callback::handle_panic
             at ./src/callback.rs:245:24
  14: test_getter_setter::<impl pyo3::class::impl_::PyMethods<test_getter_setter::ClassWithProperties> for pyo3::class::impl_::PyClassImplCollector<test_getter_setter::ClassWithProperties>>::py_methods::METHODS::__wrap
             at ./tests/test_getter_setter.rs:12:1
  15: _PyObject_GenericSetAttrWithDict
  16: PyObject_SetAttr
  17: _PyEval_EvalFrameDefault
  18: <unknown>
  19: _PyEval_EvalCodeWithName
  20: PyEval_EvalCodeEx
  21: PyEval_EvalCode
  22: pyo3::python::Python::run_code
             at ./src/python.rs:398:27
  23: pyo3::python::Python::run
             at ./src/python.rs:362:19
  24: test_getter_setter::class_with_properties
             at ./tests/test_getter_setter.rs:62:5
  25: test_getter_setter::class_with_properties::{{closure}}
             at ./tests/test_getter_setter.rs:46:1
  26: core::ops::function::FnOnce::call_once
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/ops/function.rs:227:5
  27: core::ops::function::FnOnce::call_once
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/ops/function.rs:227:5

I believe the problem resides in the implementation of [setter]:

pyo3::class::PyMethodDefType::Setter({
            #deprecations
            pyo3::class::PySetterDef::new(
                #python_name,
                pyo3::class::methods::PySetter({
                    unsafe extern "C" fn __wrap(
                        _slf: *mut pyo3::ffi::PyObject,
                        _value: *mut pyo3::ffi::PyObject,
                        _: *mut std::os::raw::c_void
                    ) -> std::os::raw::c_int {
                        pyo3::callback::handle_panic(|_py| {
                            #slf
                            let _value = _py.from_borrowed_ptr::<pyo3::types::PyAny>(_value);
                            let _val = pyo3::FromPyObject::extract(_value)?;

                            pyo3::callback::convert(_py, #setter_impl)
                        })
                    }
                    __wrap
                }),
                #doc
            )
        })

__wrap here ultimately seems to become the value for a PyTypeObject.tp_getset entry PyGetSetDef.set. From the official docs (https://docs.python.org/3/c-api/structures.html#c.PyGetSetDef):

set functions take two PyObject* parameters (the instance and the value to be set) and a function pointer (the associated closure):

typedef int (*setter)(PyObject *, PyObject *, void *);

In case the attribute should be deleted the second parameter is NULL. Should return 0 on success or -1 with a set exception on failure.

The important takeaway here is the value can be NULL if a del/__delattr__ operation is being performed.

However, the implementation calls from_borrowed_ptr, which panics on NULL, leading to the error reported in this issue.

This [setter] generated code needs to accommodate NULL values to reflect a deletion. For comparison, the cpython crate handles this by passing Option<T> to setter function, where None represents the NULL value case / deletion request (http://dgrunwald.github.io/rust-cpython/doc/cpython/macro.py_class.html#properties). Ideally PyO3 would adopt that convention as well. But this is obviously backwards incompatible, since existing #[setter] implementation may use Option<T> arguments to coerce a Python None/<other type> into a Rust Option<T>. (You would need to change this to Option<Option<T>> where the outer Option represents the presence of a value and the inner Option<T> representing a None-able Python value.

Here's one potential idea for fixing this with minimal backwards compatibility concerns:

1. Change the default PySetter wrapper to raise AttributeError on _value.is_null(). (The exact exception type can be bikeshedded but I think AttributeError is appropriate.)
2. Define a new [deleter] (or similar) annotation for [pymethods] that allows defining a custom Rust function to handle attribute deletions. If present, the auto-generated PySetter wrapper would call this function on _value.is_null() instead of raising an exception.

This is a bit complex, but it does get the job done.

Other potential solutions entail arguments to [setter] to control behavior. Perhaps [setter(optional)] to opt in to a mode that handles NULL and passes Option<T> to the Rust function?

[davidhewitt]

Yikes! I think your proposal to resolve with minimal backwards-compatibility is the correct approach. Let's implement the two bullets as separate PRs.

In particular I think raising AttributeError on null value is correct fix for now, and we can implement ASAP to avoid the crash. I think we can change .from_borrowed_ptr(_value) to .from_borrowed_ptr_or_opt(_value).ok_or_else(|| PyAttributeError::new_err("can't delete attribute")?.

Adding the #[deleter] annotation seems like a good idea, though I suspect the implementation will be hard. #[setter(optional)] seems easier to implement however feels like a more confusing API for users, so I'm not super fond of that either. Probably we need to create a new issue to discuss how best to support del. For example, how should this interact with dir / vars?

Is there a particular reason you want del support in PyOxidizer? I think in the short term lack of del can be worked around by having #[pyo3(get, set)] on an Option<T> field and just get the Python user to write foo.x = None instead of del foo.x.

[mejrs]

I think in the short term lack of del can be worked around by having #[pyo3(get, set)] on an Option field and just get the Python user to write foo.x = None instead of del foo.x.

They should be able to use del foo.x. if all of that is (manually) implemented inside a __delattr__ dunder, right? Is that something we could provide a default implementation for?

[indygreg]

I don't rely on del in PyOxidizer. I just happened to have tests for it that started crashing when I ported to PyO3.

Also, since we're using tp_getset, I think that takes precedence over __delattr__. Or maybe it is the other way around. I remember there being some wonkiness with how attributes work on types defined via the C API (versus by Python) where some type slots are flat out ignored if other similar type slots are defined. I don't think you run into this issue for Python-defined types because only a single logical type slot handler for attributes is defined (and I think it is the handler that uses __dict__?).

Anyway, changing to .from_borrowed_ptr_or_opt(_value).ok_or_else(|| PyAttributeError::new_err("can't delete attribute")? should be sufficient for PyOxidizer: I'll just change my tests accordingly. We can hash out proper del support in another issue I suppose.