Deprecate Py::into_ref

[LilyFirefly]

There's actually not much code that uses Py::into_ref left and almost all of it is easy to clean up.

[LilyFirefly]

This is the only place I found that is tricky. The problem here is that Bound<'py, PyBaseException> doesn't implement std::error::Error.

I think the solution will be to update the impl_exception_boilerplate macro, but I suspect the naïve approach will run into the same orphan rules problem that I ran into with implementing std::convert::From in #3820 (comment). Though #3820 (comment) suggests we shouldn't implement std::error::Error here, so I'm not certain what the new way to say this is.

[codspeed-hq]

CodSpeed Performance Report

Merging #3867 will improve performances by 18.52%

_{Comparing LilyFoote:deprecate-py-into-ref (f8e5d21) with main (4ce9c35)}

Summary

⚡ 2 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	LilyFoote:deprecate-py-into-ref	Change
⚡	mapping_from_dict	355.6 ns	300 ns	+18.52%
⚡	sequence_from_list	355.6 ns	300 ns	+18.52%

[davidhewitt]

Thanks for this, great to see we've got to this point!

It's interesting to see the many .as_any() calls which need to be added, I think there's a papercut there which is probably a consequence of the way I prototyped Bound originally. Let's review that...

Otherwise, just a few small suggestions...

[davidhewitt]

These .as_any() calls are necessary, because the Deref implementation didn't work? Might be worth checking that, I wonder if we need to adjust.

[LilyFirefly]

Yeah, we get errors like:

error[E0599]: no method named `getattr` found for struct `pyo3::Bound` in the current scope
  --> tests/test_proto_methods.rs:85:18
   |
83 | /             example_py
84 | |                 //.as_any()
85 | |                 .getattr("value")
   | |                 -^^^^^^^ method not found in `Bound<'_, ExampleClass>`
   | |_________________|
   | 

For more information about this error, try `rustc --explain E0599`.

I ran into needing loads of as_any() when looking at deprecating Py::as_ref too.

[davidhewitt]

Hmm. I think probably we need to adjust the bounds of the Deref implementation so that it works in this case, I've added that to the tracking issue. If you have an idea, feel free to open a PR. Otherwise, I'll sleep on it and see what ideas I have tomorrow.

[LilyFirefly]

I'm wondering if we can solve this by implementing AsRef<PyAny> in the #[pyclass] proc macro. That would make the existing Deref bounds sufficient.

[Icxolu]

I have noticed this too recently. I believe this happens only if T in Bound<T> is a #[pyclass] type. I don't think implementing AsRef<PyAny> is the right path here. I believe the current bound is wrong, because we're not using the AsRef functionality in any way. It just happens to work (mostly) because we're implementing that for the native types, but it feels like we're misusing it as a marker.

I believe an explicit AsRef might still be usefull, but it would need to be AsRef<Bound<PyAny>>, so I think the Deref bound could already be Bound<'py, T>: AsRef<Bound<'py, PyAny>>,.

Then we would need to find a way to correct the impl<'py, T> AsRef<Bound<'py, PyAny>> for Bound<'py, T> blanket implementation, because it uses the same hack. This can sadly not be implemented by the macro code because of orphan rules, so maybe using a marker trait together with a blanket implementation is a path that we can take.

[davidhewitt]

Yes, I think that's the right solution too. I haven't thought of a nice form for the marker trait, presumably it would need to be implemented for basically all T: PyTypeInfo except for PyAny.

[davidhewitt]

A possibly nice idea for the marker trait is something like HasPyBaseType (as PyAny doesn't have a base), but that might imply slightly more complex structure than what we really need here.