feat: add #[pyo3(get, set)] for Cell

[AntoineRR]

This fixes #2659.
The types for which #[pyo3(get, set)] should now work are Cell, Arc and Box.

There is one issue regarding Box, the implementation of FromPyObject conflicts with another one. I could not find what the issue was, especially since the other implementations for Arc and Cell work as expected. The related code and test has been commented out for now. Maybe someone could help me fix this issue if I don't figure it out myself? There is also the possibility to remove the implementation for Box of course.

[adamreichold]

The Clone bound is not necessary IIUC, e.g.

impl<T: IntoPy<PyObject>> IntoPy<PyObject> for Box<T> {
    fn into_py(self, py: Python<'_>) -> PyObject {
        (*self).into_py(py)
    }
}

[adamreichold]

I think the coherence error is correct insofar Box<T> is a "fundamental" type IIRC, i.e. downstream crates can impl PyClass for Box<T> with the same coherence rules as if they implemented impl PyClass for T.

I am not sure whether we can get around this without introducer said additional trait to decouple FromPyObject's task to "produce a T given a PyAny" from the new (sealed) trait's "I consider this a transparent wrapper of its inner value get/set purposes", so maybe

trait PyPropertyWrapper<T>: Sealed {
  fn set(val: T) -> Self;
  fn get(&self) -> T;
}

?

[AntoineRR]

Thanks for letting me know about fundamental types, this explains the issue with Box!

[mejrs]

I think the Arc/Box impls are a bad idea. In all cases I've seen where people stumbled upon their lack of impls this would not do what people expect, for example if you have a binary tree like in #2839, then #[pyo3(get)] will just make a full clone of the field. IMO it would be better to not have this impl so people are forced to think about what they actually want.

[kngwyu]

I also feel like Box are often used with complex recurrent design patterns, so I'm not sure we can recommend exposing it in this way. ToPyObject implies that the conversion can be expensive so it should be OK, though.

[davidhewitt]

I have to agree with the reservations about Box and Arc. Similar to @adamreichold's proposed PyPropertyWrapper above, I think there's also a third member of get/set, which I've been thinking in my head as bind.

The thought is that for e.g. Arc<T: PyClass>, maybe rather than cloning into a new Python object we could make a new Python object which just stores Arc<T> and so shares ownership between Python and Rust. So I'm a bit wary of trying to go too far down this road with Box and Arc without understanding how that option might work out.

For Cell, I'm reasonably comfortable that adding this as-is will be an improvement.

[AntoineRR]

Thanks everyone for your feedback! I didn't foresee the potentials flaws of implementing IntoPy for Box and Arc, I will remove those from the PR.
I think I understand the general idea behind the PyPropertyWrapper trait although I may need more explanations (especially on bind) to try to implement this. Maybe this trait introduction should go in an other PR if we keep the IntoPy implementation for Cell?
I also thought about other types, but I think they do not fit for an IntoPy impl : RefCell (because borrow can panic), Mutex (because lock can block), and Rc (because it is not thread safe). If you have other types in mind, please let me know.

[adamreichold]

Maybe this trait introduction should go in an other PR if we keep the IntoPy implementation for Cell?

Agreed. I think the Cell additions is the completely uncontroversial and definitely useful part here. Let's finish this up before trying to start into more ambitious directions.

[davidhewitt]

bors r+

[bors]

Build succeeded:

• conclusion