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

Add IntoPyObject & FromPyObject for Arc<T> #4987

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bschoenmaeckers
Copy link
Member

@bschoenmaeckers bschoenmaeckers commented Mar 17, 2025

Closes #4887.

This PR adds IntoPyObject and FromPyObject for Arc by forwarding the conversion to &T. Since we can’t access T, we work around the naming of associated types by adding extra type parameters. This fixes the conversion error when using #[pyclass(get)] with Arc fields.

@bschoenmaeckers bschoenmaeckers force-pushed the arc-conversions branch 4 times, most recently from 6d0976c to 15b3e13 Compare March 17, 2025 15:37
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Interesting workaround. I wonder whether the additional type parameters have any impact on type inference. Did you perhaps tested/noticed anything in that regard? Maybe with the derive macro?

Also: Is it sufficiently clear that a roundtrip with Arc will not give the same Arc? Or should we document that somewhere?

@bschoenmaeckers
Copy link
Member Author

bschoenmaeckers commented Mar 17, 2025

I wonder whether the additional type parameters have any impact on type inference. Did you perhaps tested/noticed anything in that regard? Maybe with the derive macro?

I didn't experience any weird inference behaviour when writing the existing test. The new macro tests behaves fine so I think we are good there as well.

Is it sufficiently clear that a roundtrip with Arc will not give the same Arc? Or should we document that somewhere?

I am not sure if it needs explicit documentation, as all roundtrip conversions create new objects, why would this one differ? That said documentation doesn't hurt nobody.

@davidhewitt
Copy link
Member

One alternative possibility here is that we change the pyclass internals so that instead of only being able to store T they can also store a range of different containers which deref to T.

I imagine mostly that would be Arc and Box but maybe also more complicated designs where the T is actually stored as data on a separate python object and the data is just borrowed from there.

@bschoenmaeckers
Copy link
Member Author

I imagine mostly that would be Arc and Box but maybe also more complicated designs where the T is actually stored as data on a separate python object and the data is just borrowed from there.

I do not fully understand your second case. Can you give me a example of a class definition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support IntoPyObject for Arc in pyclass for pyo3(get)
3 participants