Implement `IntoPyObject for Result<T, E> where E: Into<PyErr>`

[bschoenmaeckers]

It would be nice to have this implemented so one could use PyList::new, PyTuple::new & PySet::new with fallible iterators. One quick look into the source code showed me that the callback conversion code needs adjusting to make this possible (if possible at all).

Does someone have an idea if this is feasible or wanted at all before I jump in this rabbit hole to implement this?

Thanks in advance.

[davidhewitt]

That's a very smart idea, and having support for fallible iterators would be a nice usability improvement.

I also was wondering about having FromIterator for these types, which would allow for fallibility that way. That might be an alternative to the additional impl on IntoPyObject.

Another interesting improvement (internally) by having this proposed implementation for results is that we might be able to remove a lot of complex ok wrapping (probably in 0.25 when the removal of IntoPy is completed). Ultimately this isn't necessary though for end user experience so shouldn't affect the decision.

A downside of this implementation would be that users can do weird stuff like pass results to dict.set_item which might then immediately fail if the result is Err. I'm not sure how I feel about that.

Would love to hear others' opinions and thoughts on this one.

[bschoenmaeckers]

I also was wondering about having FromIterator for these types, which would allow for fallibility that way. That might be an alternative to the additional impl on IntoPyObject.

Its a shame that FromIterator<A> for PyResult<Bound<'py, PyList>> cannot be implemented because of the orphan rule. I otherwise do not see a way for FromIterator to be fallible without panicking.

[Icxolu]

A downside of this implementation would be that users can do weird stuff like pass results to dict.set_item which might then immediately fail if the result is Err. I'm not sure how I feel about that.

This leaves very mixed feelings for me as well. I think it's quite valuable that you are forced to deal with the error. Sure most of the time it will just be bubbled up, but that's a deliberate choice and clearly visible in the code. If we allow passing the results directly this can easily happen by accident. And because there is nothing in the code distinguishing it from the infallible case, it makes it pretty hard to discover cases that return early/may deserve manual error handling.

[bschoenmaeckers]

An other option could be to let the new methods accept a IntoPyCollection (name to be bikeshed) trait and implement it for both the current iterator form and a fallible one.

[davidhewitt]

That seems potentially viable to explore. I was reminded the other day that currently PyTuple::new only works well for homogeneous tuples; the easiest way to create an inhomogeneous tuple is e.g. (1, "hello").into_pyobject(). A trait might be able to close that inconsistency.

The downside of a trait abstraction is that it's harder for new users to figure out how to use the constructor. We should weigh this up against just having multiple methods - what is easier to document and to read?

[bschoenmaeckers]

An other option is to implement impl<'py, A> FromIterator<Bound<'py, A>> for Py<PyList> instead of Bound so we don't have to rely on the lifetime of a bound item in the iterator. But this slower than going through the new method because it will add items one by one without allocating enough memory for all items in one go.

[davidhewitt]

For fallible iterators, I wonder if a good enough solution is to use Itertools::process_results, i.e.:

let iter = /* some fallible iterator */;
let list = iter.process_results(|infallible| PyList::new(py, infallible))??;

... this would probably require relaxing the ExactSizeIterator bound as per #5822 (comment)

[bschoenmaeckers]

For fallible iterators, I wonder if a good enough solution is to use Itertools::process_results, i.e.:

let iter = /* some fallible iterator */;
let list = iter.process_results(|infallible| PyList::new(py, infallible))??;

... this would probably require relaxing the ExactSizeIterator bound as per #5822 (comment)

I think this is a performance footgun. Because the Processresults iterator will always return a 0 lowerbound on size_hint, it will append all elements 1 by 1.