fix AsRef and Deref impls on Bound<T>

[Icxolu]

Part of #3684, following the discussion thread starting here: #3867 (comment)

Previously the AsRef and Deref implementations of Bound<T> used T: AsRef<PyAny> as generic bound. This was problematic for two reasons:

• the bound was actually not used, but just happens to be true for the natives python types that we want these impls to work on,
• the bound was not satisfied on custom #[pyclass] types, which we would like these impls to also work on as well.

This PR introduces a new marker trait (currently) called HasPyBaseType, which is implemented

• for all native python types except PyAny though pyobject_native_type_named, which conveniently had exactly this purpose,
• for all #[pyclass] types, as part of the macro expansion.

The AsRef and Deref bounds where modified to the new marker trait, which allows them to work for#[pyclass] types again.

Questions:

• Is this how we would like these to be implemented?
• Should we keep the HasPyBaseType name?
• Should the trait be public or #[doc(hidden)]

---

The second commit cleans up some .as_any() calls that needed to be introduced by the missing Deref impl.

[adamreichold]

The two inline code sections for AsRef and Deref miss a closing >.

[adamreichold]

One more fundamental question: Do we need a bound here at all? If Bound::as_any can be called without this as soon as we have an instance of Bound<T> at hand, what does the additional bound here achieve?

[Icxolu]

I believe if we don't put a bound on it we'll get infinite recursion while auto-dereferencing, because it would be also available on Bound<PyAny>.

[adamreichold]

But auto-derefercing happens only as often as necessary until a resolution can be found, so why would it dereference again after having reached Bound<PyAny> as any method that can be will have been found? And a non-existing method will not exist, but I expect the compiler to have safe guards against infinite recursion, either a hard limit or loop detection.

Or does this not compile at all? Did you try just dropping the bound?

[adamreichold]

error[E0055]: reached the recursion limit while auto-dereferencing `instance::Bound<'_, types::any::PyAny>`
  --> src/types/typeobject.rs:72:14
   |
72 |             .to_owned()
   |              ^^^^^^^^ deref recursion limit reached
   |
   = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`pyo3`)

But auto-derefercing happens only as often as necessary until a resolution can be found, so why would it dereference again after having reached Bound as any method that can be will have been found?

So this appears to be wrong.

And a non-existing method will not exist, but I expect the compiler to have safe guards against infinite recursion, either a hard limit or loop detection.

And this appears to be a hard limit defaulting to 256 steps.

[Icxolu]

I guess this hits this one: rust-lang/rust#19509

[Icxolu]

So this appears to be required for Deref. I would suggest dropping it for AsRef tough and making a code comment for the Deref impl.

That sounds good to me, I'll update that.

As for the name shed, I would add CoercePyAny following std's CoerceUnsized.

I think I'd be fine with that, but I want to note here that I would consider coercion a distinct mechanism in Rust, which is not really what's happening here. I can see the similarities, but it also could potentially cause confusion. Let's see what other think about that.

I'd suggest keeping the machinery required to implement native types public/documented as downstream crates might need to wrap other native types than those provided in the pyo3 crate, e.g. rust-numpy has PyArray<T, D> for which the existing pyobject_native_type_* macros proved insufficient to handle.

👍

[davidhewitt]

I think I'd be fine with that, but I want to note here that I would consider coercion a distinct mechanism in Rust, which is not really what's happening here. I can see the similarities, but it also could potentially cause confusion. Let's see what other think about that.

Agreed with this, CoercePyAny is reasonable but I'd sort of expect Bound<T> to coerce to Bound<PyAny> as a non-reference too if it was proper coercion. (e.g. in fn foo(any: Bound<PyAny>) pass any Bound and get the coercion.)

Some other names I can think of:

• IsNotPyAny
• BoundDerefToPyAny

[adamreichold]

I would additionally nominate DerefToPyAny then as I don't think this is limited to Bound as the outer wrapper, i.e. we could do this for Borrowed<T> as well (if it did not already Deref to Bound<T>).

(I dislike IsNotPyAny because traits should IMHO encapsulate the required properties and/or operations, not type identities. But then again, the whole bound seems to work around what I'd consider a quality of implementation issue in rustc as linked above.)

[Icxolu]

I also don't really like IsNotPyAny for the same reasons. I agree that this is in principle independent of the container type, so I have renamed it to DerefToPyAny for now. I also added a note in the docs about why this is needed and that it will be removed if the compiler gets smart enough to accept this.

[davidhewitt]

DerefToPyAny seems clear enough 👍

[davidhewitt]

This looks good to me as a solution to keep the behaviour working like it would for the GIL Refs, we seem to have three of us agree on DerefToPyAny 👍

Thanks!