Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow downstream overwrite of the Bound<T> deref target #3956

Closed
wants to merge 2 commits into from

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Mar 12, 2024

Following PyO3/rust-numpy#411 (comment) this explores the possibility to allow downstream crates that implement native Python type (like rust-numpy) to overwrite the Deref::Target.

The approach described here renames DerefToPyAny to DerefToTarget and adds a Target associated type. The blanked Deref implementation is adjusted to use that as its target inside Bound. pyo3s native types use PyAny as default, #[pyclass]es use their inherited (#[pyclass(extends = ...]) class if they have or PyAny if not.

Questions

  • Do we want to expose this functionality this way? Are there alternatives?
  • Should DerefToTarget be an unsafe trait now, that we use Target for an unsafe cast?
  • I got 3 error messages in pyo3s code base of the following type:
    error[E0055]: reached the recursion limit while auto-dereferencing `instance::Bound<'_, _>`
       --> src/types/frozenset.rs:313:23
        |
    313 |         let ptr = set.as_ptr();
        |                       ^^^^^^ deref recursion limit reached
        |
        = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`pyo3`)
    
    For more information about this error, try `rustc --explain E0055`.
    
    I'm not sure why they are triggered, but they could all be fixed using simple type annotations. There is a possibility that this could also be triggered in downstream code and the error message is not really that helpful.... I quickly tested this against my migration branch of rust-numpy, it didn't cause anything there.

@Icxolu Icxolu added the CI-skip-changelog Skip checking changelog entry label Mar 12, 2024
@davidhewitt
Copy link
Member

Thanks for testing this out! It feels to me like something of this form is desirable, but ouch, that recursion limit error is painful.

Do we want to expose this functionality this way? Are there alternatives?

So I think while Deref is quite close to what we want, actually I think a future Rust might be better served by something like a beefed-up CoerceUnsized. You could imagine that if all these PyO3 smart pointers implemented coercion to base types, which could be even more powerful than Deref as it could coerce and transfer ownership, rather than only work for references.

There's another potential sticking point here that Python multiple inheritance isn't really represented by a Deref chain. I'm not sure if we could easily express multiple inheritance chains via traits anyway. It's not something PyO3 has tried to support at all thus far.

Possible prior art is that wasm_bindgen uses Deref to model inheritance: https://rustwasm.github.io/wasm-bindgen/web-sys/inheritance.html#accessing-parent-classes-using-deref

Should DerefToTarget be an unsafe trait now, that we use Target for an unsafe cast?

I guess so, yes.

I'm not sure why they are triggered, but they could all be fixed using simple type annotations.

If I had to guess, it's something like when the T of the bound is free to be inferred, the compiler wants to try to solve the .as_ptr() candidates by assuming that the Deref implementation does apply (i.e. T: DerefToTarget) and traverses it. That then yields T::Target which is also unknown type, and the compiler continues to assume Deref applies and goes into an infinitely deep rabbit hole. I don't know if we should ask compiler folks if this is expected behaviour.

I had a look at https://rustc-dev-guide.rust-lang.org/method-lookup.html


So, having looked at this, I'm slightly wary that this adds some papercuts in the Deref recursion trap. Also the fact that this might make it harder for us to do multiple inheritance makes me wonder if this isn't the right approach after all. A shame, because if it worked well it could have been an elegant way to represent linear inheritance chains...

@Icxolu
Copy link
Contributor Author

Icxolu commented Mar 13, 2024

but ouch, that recursion limit error is painful.

Yeah, that error is just awful. If I would hit that downstream (without knowing about pyo3s internals), I would have no idea what's happening.

like a beefed-up CoerceUnsized

A custom type coercion would be really cool and potentially open up a lot of interesting use cases.

Possible prior art is that wasm_bindgen uses Deref to model inheritance

It looks like they get away with it, because they don't seem have a generic deref implementation. And since they have a closed number of types, they can do that. Not really something that can be used here unfortunately.

For our case it could be better to just reuse the ...Methods traits, that downstream crates need anyway, to make methods available on multiple Bound<T>, like I started in rust-numpy (at least for now)

I had a look at https://rustc-dev-guide.rust-lang.org/method-lookup.html

Thanks for that, I think you are right. This "This is done by progressively dereferencing the receiver type until it cannot be deref'd anymore" seems to be the root cause. So it's really the same problem that we had when initially implementing Deref, just even more obscure due to type inference.

With these drawbacks I think I agree that this may not be the way to continue. It just feels not really robust and could break very easily.

@Icxolu Icxolu closed this Mar 15, 2024
@Icxolu Icxolu deleted the deref-overwrite branch April 1, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants