Replace (A)IterNextOutput by autoref-based specialization to allow returning arbitrary value

[adamreichold]

To be done:

• changelog entry
• migration guide entry

Closes #3190

[davidhewitt]

Fantastic! I am a little tight for time today, I'll do my best to give this a full read tomorrow 👍

[davidhewitt]

I've had a read and makes sense, I'll hold off from too many comments on the diff while we're figuring out the design.

I would like to suggest we don't deprecate the Option<T> special case. As per #3190 (comment) I think we can use the None variant for efficiency of returning NULL. I also think the fact that Rust iterators return Option will make this specialization very attractive in user code.

And I guess if we did keep that, probably PyResult<Option<T>> also needs to specialise in the same way? Rust fallible iterators of Option<Result<...>> can probably then just .transpose() away the error. Or would we want to specialise Option<Result<..>> directly? 🤔

[adamreichold]

I would like to suggest we don't deprecate the Option special case.

Yeah, I was leaning into direction as well after seeing how little changes are necessary to make this work for existing code. And removing IterNextOutput completely should ensure compilation errors where semantics need to be adjusted.

As per #3190 (comment) I think we can use the None variant for efficiency of returning NULL.

If I understand this correctly, I would need to remove the generic Target and enforce *mut ffi::PyObject for that to work which should work since the specialization is used only for this single slot type?

And I guess if we did keep that, probably PyResult<Option> also needs to specialise in the same way? Rust fallible iterators of Option<Result<...>> can probably then just .transpose() away the error. Or would we want to specialise Option<Result<..>> directly? 🤔

I already added PyResult<Option<T>>. I think adding Option<Result<..>> would require a second layer of specialization and be even harder to explain, so I would like to avoid adding it and defer Rust users to transpose (which suitable guide entries). Especially since even the conversion trait bounds themselves will be a bit messy due to the need to convert the error type as most iterators will probably not yield PyErr directly.

[davidhewitt]

If you are not fond of specialization introducing subtle behavioural difference based on return type, maybe rather than specialize on Option<T> we could have our own enum to specialize on, with handy traits to convert options and options-of-results to it? (And deprecate the Option<T> pathway as you currently have done here)

[davidhewitt]

If I understand this correctly, I would need to remove the generic Target and enforce *mut ffi::PyObject for that to work which should work since the specialization is used only for this single slot type?

I think so, yes. Given the tags only work for this slot, I can't see a potential downside right now, and as a performance optimization I guess we can back out again if it's problematic.

[davidhewitt]

I already added PyResult<Option<T>>. I think adding Option<Result<..>> would require a second layer of specialization and be even harder to explain, so I would like to avoid adding it and defer Rust users to transpose (which suitable guide entries). Especially since even the conversion trait bounds themselves will be a bit messy due to the need to convert the error type as most iterators will probably not yield PyErr directly.

👍 Makes sense to me!

[adamreichold]

If you are not fond of specialization introducing subtle behavioural difference based on return type, maybe rather than specialize on Option<T> we could have our own enum to specialize on, with handy traits to convert options and options-of-results to it? (And deprecate the Option<T> pathway as you currently have done here)

Yes, I am not fond of that. But the resulting usability and flexibility is a strong argument for supporting Option directly in this manner. And with really one special case, I think it is mainly a question of documentation which I will need to overhaul in any case after we settled design and implementation.

[adamreichold]

Turns out I did not fully wire the holders change through. Added this as a separate commit here and now the modified test case does indeed compile.

[davidhewitt]

Looking good!

[codspeed-hq]

CodSpeed Performance Report

Merging #3661 will improve performances by 22.03%

_{Comparing iter-output-type (5528895) with main (3583b9a)}

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	iter-output-type	Change
⚡	list_via_downcast	153.9 ns	126.1 ns	+22.03%

[davidhewitt]

Looks great! I have just one suggestion on wording in the guide, and also a thought on whether we can/should generalise the error type for the result specialisations.

[adamreichold]

(I guess at some point in the future, we'd want to do something similar for function arguments to avoid matching on the "Option" name?)

[davidhewitt]

Quite possibly, yes. I played around with something to that regard in the past, but I think the patch is now lost/dead. Also I believe that to completely stop matching on "Option" as a text string, we would need to proceed with #2935

[davidhewitt]

Looks fantastic, thanks! It's very pleasing to see that this makes user code read nicer :)

[gi0baro]

@davidhewitt Just noticed this and I have a question in regards to the deprecation: is there a way to yield None (also known as bare yielding) without using IterNextOutput?

For instance, how this code could be translated to avoid IterNextOutput usage?

#[pyclass]
pub(crate) struct PyIterAwaitable {
    result: Option<PyResult<PyObject>>,
}

impl PyIterAwaitable {
    pub(crate) fn new() -> Self {
        Self { result: None }
    }

    pub(crate) fn set_result(&mut self, result: PyResult<PyObject>) {
        self.result = Some(result);
    }
}

#[pymethods]
impl PyIterAwaitable {
    fn __await__(pyself: PyRef<'_, Self>) -> PyRef<'_, Self> {
        pyself
    }

    fn __iter__(pyself: PyRef<'_, Self>) -> PyRef<'_, Self> {
        pyself
    }

    fn __next__(&self, py: Python) -> PyResult<IterNextOutput<PyObject, PyObject>> {
        match &self.result {
            Some(res) => match res {
                Ok(v) => Ok(IterNextOutput::Return(v.clone_ref(py))),
                Err(err) => Err(err.clone_ref(py)),
            },
            _ => Ok(IterNextOutput::Yield(py.None())),
        }
    }
}

[davidhewitt]

@gi0baro great question!

To update, you'd want to directly return py.None() from __next__, and when you're ready to Return, raise a StopIteration error like how any other exception is raised by PyO3:

    fn __next__(&self, py: Python) -> PyResult<PyObject> {
        match &self.result {
            Some(res) => match res {
                Ok(v) => Ok(PyStopIteration::new_err(v.clone_ref(py)).into_py(py)),
                Err(err) => Err(err.clone_ref(py)),
            },
            _ => Ok(py.None().into_py(py)),
        }
    }