pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

#[pyclass] struct methods cannot move self

Open DecrepitHuman opened this issue 3 years ago • 11 comments

Bug Description

Any methods implemented on #[pyclass] structs that attempt to take ownership and move self produce type mismatch errors

Steps to Reproduce

  1. Create #[pyclass] in module
  2. Implement function taking ownership of self

Backtrace

error[E0308]: mismatched types
 ---> src/tcp/server.rs:10:1
   |
10 | #[pymethods]
   | ^^^^^^^^^^^^ expected struct `Server`, found mutable reference
   |
   = note:         expected struct `Server`
           found mutable reference `&mut PyRefMut<'_, Server>`


### Your operating system and version

Ubuntu 20.04.3 LTS

### Your Python version (`python --version`)

3.8.10

### Your Rust version (`rustc --version`)

rustc 1.59-0-nightly (23f69235a 2021-12-20)

### Your PyO3 version

0.16.1

### How did you install python? Did you use a virtualenv?

apt

### Additional Info

_No response_

DecrepitHuman avatar Mar 15 '22 12:03 DecrepitHuman

You can't take ownership of self since it's wrapped by a Python object, and that Python object can be referenced anywhere.

But the error could be better, so let's keep this open for an improvement.

birkenfeld avatar Mar 15 '22 12:03 birkenfeld

Also, the guide only says "&self and &mut self can be used" about instance methods. This should be expanded to cover the other possible receivers, and the caveats with &mut self.

birkenfeld avatar Mar 15 '22 12:03 birkenfeld

That explains it... however I'm in a situation where I need to manually drop some properties on the struct, and for that I need ownership of self. To give some context, I have either a TCP or UDP socket running and need to be able to close whatever file descriptor is created when opening each one

DecrepitHuman avatar Mar 15 '22 13:03 DecrepitHuman

however I'm in a situation where I need to manually drop some properties on the struct, and for that I need ownership of self

I suspect that you will need to do "the option dance", e.g. replace a field of type TcpStream by Option<TcpStream> so you can drop the field using Option::take using only a &mut self to the struct.

adamreichold avatar Mar 15 '22 14:03 adamreichold

Yes, or something like enum Socket { Tcp(TcpStream), Udp(UdpSocket) [, None] }

birkenfeld avatar Mar 15 '22 15:03 birkenfeld

Great, thanks for the fast responses. I appreciate it. Will keep issue open as asked above

DecrepitHuman avatar Mar 15 '22 15:03 DecrepitHuman

But the error could be better, so let's keep this open for an improvement.

This should be fairy straightforward, right? Just emit a custom error if the receiver is self?

I don't think we do anything for pyclasses that could be Copy and could have self receivers. Do we want to do that maybe?

mejrs avatar Mar 15 '22 18:03 mejrs

From https://github.com/PyO3/pyo3/pull/2238#discussion_r830675173

we could probably support moving out of self if PyCell were adjusted to support only 2^64 - 3 instead of 2^64 - 2 shared borrows thereby creating another niche that can be used for moved-from values. Thereby turning it into something like a RefCell<Option<T>>, but the ergonomics of all subsequent method invocations not explicitly taking slf: &PyCell<T> panicking would probably be less than nice.

adamreichold avatar Mar 21 '22 07:03 adamreichold

I would be wary of that. I've fought quite a few "underlying C++ object has been deleted" problems in PyQt which is the equivalent.

birkenfeld avatar Mar 21 '22 09:03 birkenfeld

Would something like

#[pyclass]
struct Foo(Option<FooInner>);

#[pymethods]
impl Foo {
  fn consume(&mut self) {
    let self_ = self.0.take().unwrap();
  }
}

work ATM? If so, maybe this would be a helpful pattern to include in the guide?

adamreichold avatar Mar 21 '22 10:03 adamreichold

Yeah I personally use that to avoid copying in my __iter__ implementations.

#[pyclass(name = "Index")]
pub struct PyCacheIndex {
    inner: Option<CacheIndex<Initial>>,
}

#[pymethods]
impl PyCacheIndex {
     fn __iter__(&mut self, py: Python) -> PyResult<Py<PyCacheIndexIter>> {
        let inner = std::mem::take(&mut self.inner);
        let inner = inner
            .ok_or_else(|| PyReferenceError::new_err("CacheIndex is not available after using `iter()`"))?
            .into_iter();

        let iter = PyCacheIndexIter { inner };
        Py::new(py, iter)
    }
}

mejrs avatar Mar 21 '22 10:03 mejrs

I'm going to close this as done. I think #2238 improved the experience for users significantly, and I don't think we're planning to modify PyO3 to support this any time soon.

davidhewitt avatar Feb 15 '23 06:02 davidhewitt