refactor OkWrap to not call .into_py(py)

[davidhewitt]

Motivated by https://github.com/PyO3/pyo3/pull/3588/files#r1403933922

The goal here is to keep the OkWrap trait as simple as it can possibly be.

@wyfo what do you think? It conflicts a little bit with #3588, so that'd need rebasing if this merged, but I like how this keeps OkWrap simple.

[wyfo]

I didn't dare to do this refactor as part of the async work, but I fully agree with this simplification

[wyfo]

Suggested change

	pub fn map_result_into_ptr<T: IntoPy<PyObject>>(
	py: Python<'_>,
	result: PyResult<T>,
	) -> PyResult<*mut ffi::PyObject> {
	result.map(|obj| obj.into_py(py).into_ptr())
	}
	/// This is a follow-up function to `OkWrap::wrap` that converts the result into
	/// a safe wrapper.
	pub fn map_result_into_py<T: IntoPy<PyObject>>(
	py: Python<'_>,
	result: PyResult<T>,
	) -> PyResult<PyObject> {
	result.map(|err| err.into_py(py))
	}
	pub fn map_result_into_ptr<T, E>(
	py: Python<'_>,
	result: Result<T, E>,
	) -> PyResult<*mut ffi::PyObject>
	where
	T: IntoPy<PyObject>,
	PyErr: From<E>,
	{
	Ok(result?.into_py(py).into_ptr())
	}
	/// This is a follow-up function to `OkWrap::wrap` that converts the result into
	/// a safe wrapper.
	pub fn map_result_into_py<T, E>(
	py: Python<'_>,
	result: Result<T, E>,
	) -> PyResult<PyObject>
	where
	T: IntoPy<PyObject>,
	PyErr: From<E>,
	{
	Ok(result?.into_py(py))
	}

[davidhewitt]

I guess it's a bit of a style thing, I slightly prefer the combinators over ? in these one-line implementations.

[wyfo]

The suggestion is not about the ? but using Result<T, E> instead of PyResult<T>. If you use map_err instead of ?, you can in fact relax the bound to E: Into<PyErr> instead of PyErr: From<E>, and that's maybe better (I should also relax the bound in async support implementation by the way)

[davidhewitt]

Ah I see. I take your point (and I did initially accept Result<T, E>) but having the E: Into<PyErr> bound introduced on this function and doing .map_err in here instead of directly in the macro generated code means this internal function map_result_into_ptr appears in the compiler error.

I didn't think that was useful (that was the UI test regression noted in the comment below) so I kept with just PyResult for now as all we need.

I nearly changed the async support but actually I think it's better if I don't touch that in this PR so that it conflicts less with what you've got stacked up.

[wyfo]

[...] means this internal function map_result_into_ptr appears in the compiler error.

Makes sense indeed.

[wyfo]

Why this modification?

[davidhewitt]

It's a bit of a drive-by but I was working on macOS today and I broke CI in the first attempt because this UI test regressed. Turns out the macos and linux outputs are the same.

[mejrs]

(I think) I added this because the output is different on Windows. The error message for this lists all the variants, and windows has additional error classes.

[davidhewitt]

Yep that's what I recall too, which is the why I left windows in the not(any()) list

[wyfo]

👍 for me

[davidhewitt]

Thanks again for review, as this touches more code which other contributors initially helped design I'll wait to merge a little bit in case of other opinions. But not too long, as you'll need to rebase off this 🙂