Add FromIterator for Py<PyList>

[bschoenmaeckers]

closes #4782

[Icxolu]

I generally think we should avoid impls that internally need to call Python::attach. It's surprising and not really visible to the user and thus can easily lead to deadlocks. For the same reason we also removed the Clone impl of Py.

Adding this to Bound<PyList> could be an option however.

[davidhewitt]

Another option is to require the item type to be Bound<'py, PyAny> - that way the items guarantee the thread is attached.

That requires callers to do the work with into_pyobject(), but that's possibly a good thing anyway because it would avoid the need for the Infallible constraint.

[Icxolu]

Great idea, that also works 👍

[bschoenmaeckers]

I was also thinking about this. But I was concerned it might let users sneak out a python token using a empty iterator.

[davidhewitt]

Ah, great catch. The following indeed segfaults:

impl<'py> FromIterator<Bound<'py, PyAny>> for Bound<'py, PyList> {
    fn from_iter<T: IntoIterator<Item = Bound<'py, PyAny>>>(_iter: T) -> Self {
        PyList::empty(unsafe { Python::assume_attached() })
    }
}

#[test]
fn test_fabricate_list() {
    std::iter::empty().collect::<Bound<'_, PyList>>();
}

It's interesting to conclude that Python::assume_attached() is unsound in methods that take optional bound values - I think I had not previously realised this, and assumed the presence of the 'py lifetime in Option<Bound<'py, PyAny>> was enough to create soundness.

... searching the codebase briefly, I didn't spot any places where this is currently an issue.

[bschoenmaeckers]

I could panic (maybe only in debug?) when a user tries to call this without a attached thread. And by only implementing on Py<PyList> we prevent leaking an unbounded python token. Would this be sufficient?

impl<'py> FromIterator<Bound<'py, PyAny>> for Py<PyList>
{
    fn from_iter<T: IntoIterator<Item = Bound<'py, PyAny>>>(iter: T) -> Self {
        let mut elements = iter.into_iter().peekable();
        let py = elements.peek().map_or_else(|| {
            assert!(thread_is_attached());
            unsafe { Python::assume_attached() }
        }, |item| item.py());

        PyList::new(py, elements)
            .expect("Could not allocate enough memory to add elements to the list")
            .unbind()
    }
}

[bschoenmaeckers]

I think that adding the Infallible constraint would make this more flexible and makes it more obvious that this function does not unwrap conversion failures. What do you think?

impl<'py, A> FromIterator<Bound<'py, A>> for Py<PyList>
where
    Bound<'py, A>: IntoPyObject<'py, Error = Infallible>,

[bschoenmaeckers]

I'm also fine with the following:

impl<'py, A> FromIterator<Bound<'py, A>> for Py<PyList>
where
    Bound<'py, A>: IntoPyObject<'py>,

[Icxolu]

I have to say that personally I'm not convinced that we should add this under these circumstances. Just using PyList::new directly seems like the cleaner solution to me.

[davidhewitt]

I think I'm coming to the same conclusion, especially after #4782 (comment) which we could document as an option for fallible iterators

[davidhewitt]

We should consider is relaxing the need for PyList::new to receive ExactSizeIterator, otherwise this implementation provides a (possibly unexpected) way to get around that.

Not sure of the pros/cons to doing that.

[bschoenmaeckers]

I would love to relax the bound on PyList::new but this will make it impossible to implement the upper bound check that is currently in place. try_as_dyn would solve this in the future.

[Tpt]

Can we just drop this check and do the same thing as your FromIterator implementation by just calling append on the extra elements? This way we provide a more usable API and avoid the try_as_dyn requirement

[bschoenmaeckers]

We could drop the check, but let's see if other maintainers think that is desirable.

[davidhewitt]

I think calling append on the extra elements is relatively fine (it's what Vec would do, after all), however I think the bigger downside is when there's fewer elements in the iterator than expected.

Because we have to set the list to the expected size to the list up-front, we'd need to "delete" the invalid entries off the end of the list, possibly first pre-filling with a sentinel value such as None.

I'm open to seeing how that looks, however that's probably fiddly enough it's worth doing in its own PR separate to this one.

I also am not sure if we can do similar for PyTuple (if we even should).

[bschoenmaeckers]

Can't we just trust the lower bound on size_hint and panic if the iterator produces less than that?

[davidhewitt]

I guess that can work 👍

[davidhewitt]

Another option is to require the item type to be Bound<'py, PyAny> - that way the items guarantee the thread is attached.

That requires callers to do the work with into_pyobject(), but that's possibly a good thing anyway because it would avoid the need for the Infallible constraint.