Remove ManagedPyRef

[davidhewitt]

I've been looking at ManagedPyRef<T> to understand how we can fix it for specialization.

I think that a concept like this is nice, but in its current form I don't see a way that we can make this work well on stable Rust. It uses AsPyPointer trait to specialize on. min_specialization feature doesn't allow specializing on traits so I think it could yet be many years before the current design is sound / stable.

As well as this, there is a lifetime safety issue with using ManagedPyRef<T> with PyObject (or Py<T>) that leads to dangling objects. The following code compiles in current master:

let gil = Python::acquire_gil();
let py = gil.python();
let object = {
    let _pool = unsafe { py.new_pool() };
    PyDict::new(py).to_object(py)
};

// Create ManagedPyRef
let borrowed = ManagedPyRef::from_to_pyobject(py, &object);

// Dropping object does not end lifetime of borrowed
drop(object);

// Use of dangling object
println!("refcount: {:?}", unsafe { ffi::Py_REFCNT(borrowed.as_ptr()) });

The snippet above prints refcount: 0, so clearly if I tried to pass this object to any other Python FFI I'll get a crash.

I think there's ways to resolve this lifetime issue, but it still leaves me concerned that ManagedPyRef as implemented is probably not quite the right design.

Also, we have the ToBorrowedObject trait which serves a very similar function but does not have this lifetime issue.

Because it's a blocker for us getting onto stable Rust, I would like to suggest that for now we remove it. I would like to one day perhaps re-introduce this as PyHandle<T>, but only when we can get a safe design which compiles on stable.

[kngwyu]

Thank you!

As well as this, there is a lifetime safety issue

Hmm... I don't think it is problematic since the example uses unsafe.

But I agree that ManagedPyRef is currently not used in our repository and there's no room to improve this trait.