Skip to content
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.

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

Add trait for cloning while the GIL is held #4133

Open
adamreichold opened this issue Apr 28, 2024 · 3 comments
Open

Add trait for cloning while the GIL is held #4133

adamreichold opened this issue Apr 28, 2024 · 3 comments

Comments

@adamreichold
Copy link
Member

Due to issues making delay reference count increments sound, they were removed #4095 and replaced by an Clone implementation for Py which will panic without the GIL being held, gated by the py-clone feature.

This has several usability downsides, e.g. #[pyo3(get)] stops working with Py<T> fields which however is a very important use case when sharing data with Python code. Similarly, the PyBacked* types cannot be unconditionally Clone themselves.

Both problems (and likely others) should be resolvable if we add a trait, e.g. CloneRef or PyClone with signature

fn clone_ref(&self, py: Python<'_>) -> Self;

and implement it unconditionally for Py<T> and add a blanket impl based on Clone. The proc macro machinery behind #[pyo3(get)] could then go via this trait instead of the plain Clone.

@davidhewitt
Copy link
Member

👍 I guess we probably want a derive macro for this trait too.

@adamreichold
Copy link
Member Author

I started to look into this and there are hurdles, for starters we really want the blanket impl

impl<T> PyClone for T
where
    T: Clone,
{
    fn clone_ref(&self, _py: Python<'_>) -> Self {
        self.clone()
    }
}

so that we can fall back to normal Clone in getters for types like i32.

But this conflicts with necessary impls like

impl<T> PyClone for Option<T> where T: PyClone;

impl<T> PyClone for Vec<T> where T: PyClone;

and this also does not really end because set for which we would want to provide PyClone is open-ended.

(It also conflicts with the PyClone impl for Py if the py-clone feature is enabled, but we could suppress that.)

So I don't think it is possible to make this as seamless as Clone especially considering types outside of our purview.

The only reasonable idea I currently have is to again make use of use being in macro context when we call clone in impl_py_getter_def, meaning we use autoref specialization to prefer calling clone_ref if it exists and fallback to clone otherwise. Meaning we also do not provide a blanket impl of PyClone at all and implement it for our types and some basic ones like Option and Vec.

What do people think? Hopefully anybody finds a better approach. I will really feel bad about adding yet another case of autoref specialization to our proc macros...

@davidhewitt
Copy link
Member

Uff, this kind of complexity sounds all too familiar to me.

The only reasonable idea I currently have is to again make use of use being in macro context when we call clone in impl_py_getter_def, meaning we use autoref specialization to prefer calling clone_ref if it exists and fallback to clone otherwise. Meaning we also do not provide a blanket impl of PyClone at all and implement it for our types and some basic ones like Option and Vec.

What do people think? Hopefully anybody finds a better approach. I will really feel bad about adding yet another case of autoref specialization to our proc macros...

I did indeed look into the specialization approach in #4254, and I used ToPyObject for cases like Vec<Py<T>> instead of the current IntoPy + Clone.

I at least didn't use autoref specialization and instead used type-level const logic to specialize, which might be an easier pill to swallow? 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants