fixes #3325 -- mark AsPyPointer as unsafe trait

[alex]

No description provided.

[adamreichold]

Does this suffice? Do we want to demand some relation to the &self parameter, e.g. that the resulting must be derived from and only from &self to avoid implementations returning completely unrelated pointers?

As a more general observation. Do we need this in generic context? As an alternative would an inherent PyAny::as_ptr combined with Deref<Target=PyAny> not serve the same purpose without inventing a new contract via this trait?

[alex]

Yes, this probably has a further requirement on the refcount (notably that this borrows a reference from self, whereas into shoudl be an owned reference). I'll update.

I'm not sure to what extent we could use the deref in place of this trait.

[adamreichold]

I'm not sure to what extent we could use the deref in place of this trait.

My idea is to go from &self to &PyAny via Deref<Target=PyAny> which is unambiguously safe due to the validity requirements of &PyAny. From there we can go to *mut ffi::PyObject via self.0.get() without relying on extra contracts which must be upheld by implementors.

I think the main question is whether we use this in any situation where Deref<Target=PyAny> does not apply. I would be surprised if we did tough, because returning a valid pointer to a Python object basically means I can construct a sufficiently short-lived &PyAny from that pointer.

[davidhewitt]

I think PyAny::is and also __traverse__ are the only public APIs which immediately come to my mind, but we can find nice solutions to those I'm sure.

I would definitely like to start by seeing how far adding the inherent methods to PyAny and Py<T> gets us, and see if there's much work after that to replace and deprecate the traits entirely.

[alex]

This makes sense to me. I'm not going to have time to work on it today. My suggestion would be: I think this makes sense to merge as is, and then looking to minimize and deprecate the usage of these traits is really orthagonal.

[davidhewitt]

See #3359 as a starting point.

My only reason to wait on merging this PR here is that if we can get to a point where the traits are deprecated in 0.20 then I'd prefer not to change them at all before that release.

[alex]

At this point here are the functions that take impl AsPyPointer as an argument:

• Py::is
• PyAny::is
• PyIterator::from_object (Migrate PyIterator::from_object and PyByteArray::from from AsPyPointer to &PyAny #3389)
• PyByteArray::from (Migrate PyIterator::from_object and PyByteArray::from from AsPyPointer to &PyAny #3389)
• PyVisit::call
• impl From for PyObject (Remove usage of AsPyPointer in traits for convergint to PyObject #3391)
• impl IntoPy<PyObject> for &T (Remove usage of AsPyPointer in IntoPy<PyObject> trait implementation #3393)
• impl FromPyObject for Py<T>

I think the first four can be straightly switched to something else (e.g. IntoPy<PyObject>) and then we can call the inherent method.

The conversion impls can likely be removed or replaced with impls for each of the concrete types.

PyVisit::call is the one I'm least confident about. I don't think using IntoPy<PyObject> is quite right, since we don't really want to call back to random code there, which many implementors do. Any thoughts?

[davidhewitt]

PyAny::is and Py::is don't dereference the pointer, so I believe they are safe without adding constraints to AsPyPointer. We could consider leaving the trait and use it just for them. To deprecate the trait they could be changed to take &PyAny (breaking change) or some trait implemented for &PyAny and &Py<T>.

PyIterator::from_object and PyByteArray::from should probably just take a single argument &'py PyAny rather than py: Python<'py> plus AsPyPointer. That's breaking but a consistency improvement anyway. I also suggest changing PyDict::from_sequence at the same time, which currently takes py: Python<'py> and PyObject.

Agreed re the conversion impls.

PyVisit::call will need to be usable without the GIL, so definitely can't use IntoPy<PyObject>. Because it doesn't have access to the GIL I think the only types which will be passed at that callsite will be &Py<T> and &Option<Py<T>>, so I suggest we deprecate call and add PyVisit::visit(&Py<T>) and PyVisit::visit_option(&Option<T>). (That's breaking enough that I think it warrants a deprecation of at least one release.)

[mejrs]

This pointer may also be null, like in the case of the implementation of Option<T>

[alex]

Realizing that I missed one on my original list: impl FromPyObject for Py<T>, which I'm having trouble writing without AsPyPointer, would love some thoughts.

[adamreichold]

Realizing that I missed one on my original list: impl FromPyObject for Py<T>, which I'm having trouble writing without AsPyPointer, would love some thoughts.

This handles the different between going for &T and &PyCell<T> as the target, right? Maybe is #2956 helpful to resolve this?

[alex]

It handles going from &PyAny to Py<T>, I'm not positive if that PR would help or not.

[davidhewitt]

Hmm I had a go in #3406 which looks a possible route.

[davidhewitt]

Given we're not sure about the decision in #3406, I'm beginning to warm to the idea to merging this now and releasing 0.20 with the remaining points addressed by the unsafe bound. We can then make further progress in removing this trait with the API reworks of 0.21?

[alex]

That works for me.

[davidhewitt]

Ok, let's do that. I'll create a follow up issue to finish the removal of this trait as we go.