Closed ChayimFriedman2 closed 1 month ago
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.
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.
Overall this seems like a smart idea; however I think we should note that this makes
BoundObject
anunsafe trait
, which might scare users a little bit given thatBoundObject
is very user-facing as the result ofIntoPyObject
. 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 theunsafe
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.
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 theunsafe
a little less upfront for readers.But then users can implement
IntoPyObject
withOutput = &Bound
, which will mean their types cannot be used in this optimizations, which is unfortunate. We should teach users to useBorrowed
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).
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.
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:
https://github.com/PyO3/pyo3/blob/2f5b45efa10e52a44bb3185dfeb716c2e815a4f8/src/instance.rs#L617-L639
And replace all places where
IntoPyObject<Output = &Bound>
withOutput = Borrowed
.For users, this will be a transparent change, since
Borrowed
derefs toBound
. So why do I want this?Borrowed
has a feature&Bound
doesn't have: it is layout-compatible with*mut ffi::PyObject
(and so isBound
). What that means is that a container, for example aVec
, ofBorrowed
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 aVec
and fills it with the values. For convenience, the macro accepts any value that isIntoPyObject
.Ideally, we will construct one
Vec
, and use it to store all arguments. And we need to store them twice: once because they may beBound
s 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 oneVec
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 moreVec
s 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 toBorrowed
), but given the little utility of this impl, I suggest we just remove it.