Remove unsound custom element example.

[adamreichold]

No description provided.

[davidhewitt]

Sorry I'm so behind with reviewing at the moment! For me (and future readers), could you briefly explain what was unsound about the example being removed? Apart from that, looks good to merge; thanks again!

[adamreichold]

For me (and future readers), could you briefly explain what was unsound about the example being removed?

One problem is that we do not check the element types in extract. We do check that we have an object array but that does not tell us that the elements are all of type T, i.e. Py<T> instead of Py<PyAny>. Or whether they have a uniform type at all. But even if we would check this during extract, Python code having a reference to the (underlying) array could change the type of some elements after the check leading to type confusion. Therefore, it seems that Py<PyAny> is the only safe Rust-side type for NumPy arrays containing objects.

Will add this to the commit message.

[adamreichold]

@aldanor Since this is the continuation of #257 by other means, I would be very interested in your perspective on PyArray::from_owned_object_array?

@davidhewitt The Py<T> with T: PyClass bound would prevent this from being used with native types, right? Is there are better bound that would include those? Basically so that calling ToPyObject element-wise would be safe? T: Deref<Target=PyAny>?

[davidhewitt]

The Py with T: PyClass bound would prevent this from being used with native types, right? Is there are better bound that would include those?

Yes, PyClass would prevent native types like float etc.

Does PyTypeInfo do what you need? That's one of the few traits which is implemented for pretty much all types.

[adamreichold]

Does PyTypeInfo do what you need? That's one of the few traits which is implemented for pretty much all types.

The problem is that in the current implementation I do not "need" anything from the trait as I only reinterpret memory, but I need the trait bound to ensure that this is safe. And doing should be the moral equivalent of calling .to_py_object(py) on each element.

What I do just notice is that there is a blanket on impl

impl<T> ToPyObject for Py<T>

without any bounds. So if that is the case, I should be able to remove that bound entirely here as well.

[adamreichold]

So if that is the case, I should be able to remove that bound entirely here as well.

Went for a Py<T>: ToPyObject bound which is what the trivial implementation of the method would do. In light of the blanket impl above, this is not a restriction, but it seems good to make it explicit and have the compiler fail if that blanket impl is removed/changed.

[adamreichold]

@aldanor Friendly ping w.r.t. #255 (comment)

[kngwyu]

Thanks, it's nice that we found this before releasing 😅

[aldanor]

@adamreichold (just a note that I didn't have time to re-read and re-think this again, but will do later today when I get back to computer)

[aldanor]

Looks good to me. So we enable full functionality with PyObject, plus provide an option to export a bunch of rust-side objects to numpy (as a one-way trip only) if need be.

In all seriousness, I don't think the 'aliasing' scenario we've discussed previously is that useful anyway, as using objects in numpy arrays is quite a rare thing to start with.

[aldanor]

So if that is the case, I should be able to remove that bound entirely here as well.

Went for a Py<T>: ToPyObject bound which is what the trivial implementation of the method would do. In light of the blanket impl above, this is not a restriction, but it seems good to make it explicit and have the compiler fail if that blanket impl is removed/changed.

In my opinion, this bound may only look weird in the docs (e.g. if I saw it the method signature, this would leave me scratching my head for a bit - like, why did the authors put it there?). If Py<T>: ToPyObject no longer holds true, a very large chunk of pyo3 would have to be modified or rewritten, it would be too big of a change to go unnoticed.

[adamreichold]

In my opinion, this bound may only look weird in the docs (e.g. if I saw it the method signature, this would leave me scratching my head for a bit - like, why did the authors put it there?).

Fair enough. Removed the bound.