Skip to content

Use Arbitrary Self Types instead of Py<Type>Methods traits #4885

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LilyAcorn
Copy link
Contributor

@LilyAcorn LilyAcorn commented Feb 1, 2025

For #4869.

I removed the Deref implementation for Bound<'py, T> because it conflicts with any implementation of std::ops::Receiver we need to use Arbitrary Self Types. For example:

error[E0119]: conflicting implementations of trait `std::ops::Receiver` for type `instance::Bound<'_, PyBool>`
  --> src/types/boolobject.rs:27:1
   |
27 | impl std::ops::Receiver for Bound<'_, PyBool> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `core`:
           - impl<P, T> std::ops::Receiver for P
             where <P as Deref>::Target == T, P: Deref, P: ?Sized, T: ?Sized;

I also added an implementation of PyAnyMethods for Bound<'py, T> to compensate for the removal of Deref. To get this working, I dropped the requirement that PyAnyMethods can only be implemented for sealed types. An alternative approach I think would be to just remove Deref and require .as_any() or .into_any() calls in more places.

After all of this, clippy passes on nightly on my machine (with the exception of one lint, which I think is unrelated to this PR). I can also run much of the test suite, but get an error in #[pyclass] which I currently don't understand:

error[E0599]: no method named `as_type_ptr` found for reference `&pyo3::Bound<'_, PyType>` in the current scope
 --> tests/test_field_cfg.rs:5:1
  |
5 | #[pyclass]
  | ^^^^^^^^^^
  |
  = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)
help: there is a method `as_ptr` with a similar name
  |
5 | as_ptr
  |

I'm confident this is coming from

.as_type_ptr()
but I see no reason why the .as_type_ptr method shouldn't be found, since it's implemented in
#[inline]
pub fn as_type_ptr(self: &Bound<'py, Self>) -> *mut ffi::PyTypeObject {
self.as_ptr() as *mut ffi::PyTypeObject
}

@LilyAcorn LilyAcorn added refactoring rust Pull requests that update Rust code labels Feb 1, 2025
@LilyAcorn LilyAcorn self-assigned this Feb 1, 2025
@LilyAcorn LilyAcorn added the CI-no-fail-fast If one job fails, allow the rest to keep testing label Feb 1, 2025
@LilyAcorn LilyAcorn force-pushed the arbitrary-self-types-py-any-methods branch 6 times, most recently from 4076c62 to 42045ca Compare February 2, 2025 22:35
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for running with this! I haven't managed to read in detail, can you summarise how it feels to use? I see there's a fair bit of path syntax change, that's unexpected to me.

Do you think we should be proceeding down this road? Does it seem like it's something fundamental with our design that's wrong? Or something in the self types feature which we should flag upstream before it stabilizes?

@LilyAcorn
Copy link
Contributor Author

Thanks very much for running with this! I haven't managed to read in detail, can you summarise how it feels to use? I see there's a fair bit of path syntax change, that's unexpected to me.

Currently I think there's some rough edges. The existing Deref implementation for Bound<'py, T> to Bound<'py, PyAny> is incompatible with the required std::ops::Receiver implementation(s), which leads to a lot of knock-on changes required. In addition, there's the path syntax changes - I hope that these are only required due to a bug in the not yet stabilized Arbitrary Self Types implementation, but I haven't yet verified this.

Do you think we should be proceeding down this road? Does it seem like it's something fundamental with our design that's wrong? Or something in the self types feature which we should flag upstream before it stabilizes?

I'm currently experimenting with some alternative designs to see what level of churn they cause. Once I've finished with those, I'll have a better idea. But unless these other designs work much better, I think we'll need to flag to upstream.

Arbitrary self types conflicts with the `Deref` implementation for
`Bound<'py, T> where T: DerefToPyAny`. This commit replaces that
implementation with a blanket implementation of `PyAnyMethods` instead.

Unfortunately, this greatly increases the verbosity of using
`Py_Methods` when multiple traits match. It also requires many more
calls to `as_any()` or `into_any()`.
This largely replaces the need for `Py<Type>Methods` traits, since these
methods can now be implemented directly on the type.
@LilyAcorn LilyAcorn force-pushed the arbitrary-self-types-py-any-methods branch from 42045ca to 8ac0215 Compare February 11, 2025 19:35
@LilyAcorn
Copy link
Contributor Author

I now think this is very workable. We'll need to decide how best to handle the loss of Deref, but I think there's a few reasonable options we can consider.

@LilyAcorn LilyAcorn force-pushed the arbitrary-self-types-py-any-methods branch from 002cce5 to 3f8b496 Compare February 12, 2025 00:03
@LilyAcorn LilyAcorn force-pushed the arbitrary-self-types-py-any-methods branch from 3f8b496 to c44c9db Compare February 12, 2025 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast If one job fails, allow the rest to keep testing refactoring rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants