Do not implement `BoundObject` for `&Bound`

[ChayimFriedman2]

This is mainly an internal issue, but it has to be decided before we stabilize IntoPyObject, because after that it will be a breaking change.

I request that we remove the following impl:

pyo3/src/instance.rs

Lines 617 to 639 in 2f5b45e

	impl<'a, 'py, T> BoundObject<'py, T> for &'a Bound<'py, T> {
	type Any = &'a Bound<'py, PyAny>;
	fn as_borrowed(&self) -> Borrowed<'a, 'py, T> {
	Bound::as_borrowed(self)
	}
	fn into_bound(self) -> Bound<'py, T> {
	self.clone()
	}
	fn into_any(self) -> Self::Any {
	self.as_any()
	}
	fn into_ptr(self) -> *mut ffi::PyObject {
	self.clone().into_ptr()
	}
	fn unbind(self) -> Py<T> {
	self.clone().unbind()
	}
	}

And replace all places where IntoPyObject<Output = &Bound> with Output = Borrowed.

For users, this will be a transparent change, since Borrowed derefs to Bound. So why do I want this?

Borrowed has a feature &Bound doesn't have: it is layout-compatible with *mut ffi::PyObject (and so is Bound). What that means is that a container, for example a Vec, of Borrowed can be treated as *mut *mut ffi::PyObject (an array of *mut ffi::PyObject) for the purposes of FFI.

This may sound unimportant, but this can enable speedups. For example, the pycall!() macro I'm experimenting with, we can unpack any set of objects. Internally, if the number of entries is not constant (in which case we use a stack array), the macro allocates a Vec and fills it with the values. For convenience, the macro accepts any value that is IntoPyObject.

Ideally, we will construct one Vec, and use it to store all arguments. And we need to store them twice: once because they may be Bounds and need dropping, and the other as *mut ffi::PyObject to pass to the C FFI. Ideally, these two will collapse, and we will use one Vec for both for all arguments.

There is one obstacle for that: this impl. with this impl, we cannot take IntoPyObject::Output and treat it as *mut ffi::PyObject, because it may be a &Bound, which is actually *const *mut ffi::PyObject. That forces us to use two or more Vecs when one is enough.

It can be worked around by complicating BoundObject more (adding another fn and associated type for "if this is a &Bound convert it to Borrowed), but given the little utility of this impl, I suggest we just remove it.

[davidhewitt]

Overall this seems like a smart idea; however I think we should note that this makes BoundObject an unsafe trait, which might scare users a little bit given that BoundObject is very user-facing as the result of IntoPyObject. Not necessarily a deal-breaker, just worth considering.

[davidhewitt]

Maybe we make a sealed supertrait of BoundObject, e.g. unsafe trait LayoutCompatibleWithFFIPyObject: Sealed { } or similar, if we want to make the unsafe a little less upfront for readers.

[ChayimFriedman2]

Overall this seems like a smart idea; however I think we should note that this makes BoundObject an unsafe trait, which might scare users a little bit given that BoundObject is very user-facing as the result of IntoPyObject. Not necessarily a deal-breaker, just worth considering.

BoundObject is sealed, so it doesn't matter for users if it's unsafe or not.

Maybe we make a sealed supertrait of BoundObject, e.g. unsafe trait LayoutCompatibleWithFFIPyObject: Sealed { } or similar, if we want to make the unsafe a little less upfront for readers.

But then users can implement IntoPyObject with Output = &Bound, which will mean their types cannot be used in this optimizations, which is unfortunate. We should teach users to use Borrowed instead.

[Icxolu]

This seems like a good optimization opportunity. I originally added that impl to make it easier for users to implement IntoPyObject without reference counting overhead while keeping Borrowed out of the picture (since we tried that in 0.21/0.22). We currently use it in a few places, but I think all of them can easily switch to Borrowed instead. I think with 0.23 Borrowed will show up in a few more places due to optimizations (for example #4390 also uses it), so maybe have to accept that and document clearly what it means, and how it's used.

I agree that we should mark it as unsafe if we rely on layout compatibility of all implementors.

Maybe we make a sealed supertrait of BoundObject, e.g. unsafe trait LayoutCompatibleWithFFIPyObject: Sealed { } or similar, if we want to make the unsafe a little less upfront for readers.

But then users can implement IntoPyObject with Output = &Bound, which will mean their types cannot be used in this optimizations, which is unfortunate. We should teach users to use Borrowed instead.

I think these two points don't necessarily contradict each other. We can add such a supertrait, while also removing the impl, but personally I don't think it's necessary. We can and should document the contract in the safety docs of BoundObject, but in the end users don't really need to interact with it too much. They can't implement it themselves and we can teach in the IntoPyObject docs that they can use either Bound or Borrowed as the Output type (and when each is appropriate).

[davidhewitt]

Right, I am forgetting that users will typically call .into_pyobject() on a known type and will have a concrete type that they get back; they won't need to then go via the trait to use the result.

So 👍 let's move ahead with this.