introduce PyClassGuard(Mut) as future replacement for PyRef(Mut)

[Icxolu]

Ref #4720

Introduces PyClassGuard(Mut) as future replacement for PyRef(Mut). It switches the lifetime from 'py to 'a, borrowing from the input pointer directly regardless of whether we're attached or not. In this experiment I build it around &'a Py<T> which seems to work (at least locally), let's see what CI says.

[davidhewitt]

Thanks for experimenting with this, it seems pretty promising to me.

A few initial observations as comments.

I think also the most interesting bit will be replacing extract_pyclass_ref[_mut] in the macro machinery with this new type, I think that would manifest the performance gains we saw in #4720

[davidhewitt]

I wonder whether it's worth adding to this comment some notes around how PyClassGuard is somewhat prone to errors under free-threading, and using #[pyclass(frozen)] with blocking access of something like a Mutex to internal state may be better depending on the program requirements.

[Icxolu]

I added a few words here, let me know what you think

[Icxolu]

I think this is mostly ready now. A couple of points:

• How should we introduce this? I guess we do not want to change borrow(mut) right away but to deprecate them. How should we name the new method to obtain these? borrow_guard maybe and then drop the suffix in the future (I would do the deprecation in a followup, to keep the diff small(ish) here)
• Should we provide a panicking variant? I tend to think that is a bad habit especially if ffi is involved, so IMO we should drop that. Users can easily panic themself if they really want.
• Should AsRef/AsMut really provide as_super? I think that is a bit surprising. Usually these include at least the Deref(Mut) target and then additionally other things as well. Given that we can't do both, should we change here?

[davidhewitt]

How should we introduce this? I guess we do not want to change borrow(mut) right away but to deprecate them. How should we name the new method to obtain these? borrow_guard maybe and then drop the suffix in the future (I would do the deprecation in a followup, to keep the diff small(ish) here)

One idea which struck me is maybe we can have something like #[pyclass(storage = PyCell<Self>)] to denote a container type which allows for controlled access. Obvious alternative would be #[pyclass(storage = Mutex<Self>)] for those who want locking semantics. Maybe even #[pyclass(storage = Arc<Self>)] or#[pyclass(storage = Arc<Mutex<Self>>)].

The idea would be that this could maybe replace the current (frozen) keyword:

• we can then just make borrow() and borrow_mut() return Self::Storage::Guard, and it's nonbreaking. Users just update their #[pyclass] to make use of the new guard type and that changes their methods.
• we make the default be #[pyclass(storage = Self)], i.e. frozen - but I think for now we warn if neither #[pyclass(frozen)] or #[pyclass(storage = ...)] are set.

Should we provide a panicking variant? I tend to think that is a bad habit especially if ffi is involved, so IMO we should drop that. Users can easily panic themself if they really want.

I guess it depends if we like the above semantics, i.e. .borrow() might never panic with a mutex, just block. try_borrow() with a mutex would presumably be the nonblocking form? (Not sure how that would interact with macros, we'd have to be careful to make sure the macros block if needed.)

Should AsRef/AsMut really provide as_super? I think that is a bit surprising. Usually these include at least the Deref(Mut) target and then additionally other things as well. Given that we can't do both, should we change here?

I'm ok for not including them for now, we had reasons that we couldn't do as_super a long long time ago, so the AsRef implementations were all we had. #770 (comment)

---

Also, I guess we need to apply into_super fixes to the existing PyRef and PyRefMut types? Might be a pain 🙈

[Icxolu]

One idea which struck me is maybe we can have something like #[pyclass(storage = PyCell<Self>)] to denote a container type which allows for controlled access.

That sounds like a super interesting idea!, Definitely worth exploring. Something that's unclear is how this interacts with the ability to directly extract a PyClassGuard/PyRef. I guess we can't allow that anymore if we have different storages types, that require different guards. So I think there will still be a breaking change.

Also, I guess we need to apply into_super fixes to the existing PyRef and PyRefMut types? Might be a pain 🙈

Yeah, we should probably do that. I haven't checked yet, but I have the hope that we can simply apply the same fix there, which should be relatively straight forward,

[Icxolu]

Should we wait with this until we figured #[pyclass(storage = ...)] out, or should we land this type already without changing the borrow methods or deprecating PyRef, just so it can be used within the macros?

[davidhewitt]

I think making this type internally available so we can use it as the macros is a great first step to landing it, and probably helps sidestep the problem we have with the as_super() method for now.

I'd be keen to figure out the #[pyclass(storage)] design soon too, though I guess we probably want to get FromPyObject changes done before that? 🤔

[Icxolu]

I think making this type internally available so we can use it as the macros is a great first step to landing it, and probably helps sidestep the problem we have with the as_super() method for now.

Great, I will clean this up then.

I think I have to remove the into_super fix from here again because it has the same problem from #5253 (comment), right?

I'd be keen to figure out the #[pyclass(storage)] design soon too, though I guess we probably want to get FromPyObject changes done before that? 🤔

I think it makes sense to get the FromPyObject first, just so that we don't have to think about the upcoming change all the time while exploring.

[Icxolu]

I think this should be a MVP now. I had to change the internals because I discovered some unsoundness with my previous approach. I think this version should be good now, but I would appreciate if you could double check my reasonings on the unsafe part specifically. I added a safety comment to all blocks, which hopefully make sense.

[davidhewitt]

Thanks, this is looking great! An initial wave of comments / thoughts... 👀

[davidhewitt]

What were the changes in this file motivated by? They seem somewhat reasonable but also I think if there's no special behaviour to the NonNullObject case we should just remove it? That said if there's no need to change this maybe just revert the file diff?

[Icxolu]

This from ee55a8b. I would have to check again for the exact error, but there was something like a lifetime error because the pointer extracted from the NonNull would for some reason not live long enough.

[Icxolu]

So the failing example is this. It fails on the #[pymethod] expansion.

#[pyclass]
struct DescrCounter {}

#[pymethods]
impl DescrCounter {
    fn __set__(&self, _instance: &Bound<'_, PyAny>, _new_value: &mut Self) {}
}

The relevant part is:

#[allow(non_snake_case)]
unsafe fn __pymethod___set____(
    py: ::pyo3::Python,
    _raw_slf: *mut ::pyo3::ffi::PyObject,
    arg0: *mut ::pyo3::ffi::PyObject,
    arg1: ::std::ptr::NonNull<::pyo3::ffi::PyObject>,
) -> ::pyo3::PyResult<()> {
    let _slf = _raw_slf;
    #[allow(clippy::let_unit_value)]
    let mut holder_0 = ::pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
    let mut holder_1 = ::pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
    let mut holder_2 = ::pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
    let result = DescrCounter::__set__(
        ::pyo3::impl_::extract_argument::extract_pyclass_ref::<DescrCounter>(
            unsafe { ::pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf) }.0,
            &mut holder_0,
        )?,
        {
            #[allow(unused_imports)]
            use ::pyo3::impl_::pyclass::Probe as _;
            ::pyo3::impl_::extract_argument::extract_argument::<
                _,
                { ::pyo3::impl_::pyclass::IsOption::<&Bound<'_, PyAny>>::VALUE },
            >(
                unsafe { ::pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &arg0).0 },
                &mut holder_1,
                "_instance",
            )
        }?,
        {
            #[allow(unused_imports)]
            use ::pyo3::impl_::pyclass::Probe as _;
            ::pyo3::impl_::extract_argument::extract_argument::<
                _,
                { ::pyo3::impl_::pyclass::IsOption::<&mut Self>::VALUE },
            >(
                unsafe {
                    ::pyo3::impl_::pymethods::BoundRef::ref_from_ptr(py, &arg1.as_ptr()).0
//                                 temporary value dropped while borrowed^
                },
                &mut holder_2,
                "_new_value",
            )
        }?,
    );
    ::pyo3::impl_::callback::convert(py, result)
}

[davidhewitt]

I think rather than removing the advantages of the non-null type what I'd rather we did is have a BoundRef::ref_from_non_null(py, &arg1) on that problematic line.

While it's a duplication of the ref_from_ptr API, it might be a good thing for type safety given the non-null.

In the long run I wonder if we will replace BoundRef with Borrowed more widely across the macros, probably that would benefit from the FromPyObject changes being landed first.

[Icxolu]

Agreed, I replaced that commit with one that adds these helpers

[davidhewitt]

I'm trying to understand the need for the &'holder lifetime here. I suspect it's something about &'a mut Option<PyClassGuard<'a, T>> forcing 'a to be invariant?

I wonder, to avoid the trait picking up so many lifetimes, is there a way out with GATs?

[Icxolu]

I have to think about if there is something we can do with GATs here. I would need to try again to reproduce the exact error, but tying these lifetimes together was causing problems down the line.

[Icxolu]

If we tie 'holder and 'a together, we get the following error:

9 | #[crate::pymethods]
  | ^^^^^^^^^^^^^^^^^^-
  | |                 |
  | |                 temporary value is freed at the end of this statement
  | |                 borrow might be used here, when `holder_1` is dropped and runs the destructor for type `Option<PyClassGuard<'_, Dummy>>`
  | creates a temporary value which is freed while still in use

I think this is due to 'a being the lifetime of a function argument, while holder is defines inside that function and thus dropped first, so the holder lifetime must be shorter and can't be the same as 'a

[davidhewitt]

Understood. I think let's accept this complexity here for now, I think once we make the changes to FromPyObject we might want to use Borrowed<'a, 'py, T> in these functions and it might be that we get some refactoring opportunities then.

[davidhewitt]

These methods on NonNull are super nice, I wonder if we can use them more internally instead in places where we know the pointer is non-null but just haven't (yet) told the type system that 👍

[davidhewitt]

This is looking great, I think we should ship it in 0.26 👍

Before merge, I think it would be great to adjust the changes to the NonNull stuff in the macros as per #5233 (comment), if you agree with that idea?

[davidhewitt]

This looks perfect to me, thank you for the hard work here! Looks like this makes a meaningful improvement (~5%) to our method call performance, always great to land those without breaking any user code!