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 support for type hinting objects #4917

Merged
merged 11 commits into from
Feb 19, 2025

Conversation

IvanIsCoding
Copy link
Contributor

@IvanIsCoding IvanIsCoding commented Feb 16, 2025

Related to #4849

This PR adds the pyo3-ffi bindings to type hinting objects
and then exposes them in pyo3.

I gated this for Python 3.9+ but I am not confident this is the appropriate way to doing so in PyO3. Is this the idiomatic way? I prefered to gate the whole module instead of individual methods.

@IvanIsCoding
Copy link
Contributor Author

@ngoldbaum can you have a look at this before the other maintainers?

Copy link
Contributor

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

I've purposely avoided learning much about Python type hints so I'm not sure why you'd want to use this, although I didn't read the test closely to figure that out.

There is guide/src/python-typing-hints.md in the narrative docs - can you maybe expand the content there to describe how you might use this?

@IvanIsCoding
Copy link
Contributor Author

I've purposely avoided learning much about Python type hints so I'm not sure why you'd want to use this, although I didn't read the test closely to figure that out.

There is guide/src/python-typing-hints.md in the narrative docs - can you maybe expand the content there to describe how you might use this?

I will address the comments and try to update other PyO3 documentations related to type hints. But the core motivation is making implementing __class_getitem__ easier.

@IvanIsCoding
Copy link
Contributor Author

After reading guide/src/python-typing-hints.md, I think David's comment in #4849 (comment) becomes more relevant.

__class_getitem__ is more of an implementation details, so I'd rather defer the documentation updates once in a separate PR we add a macro allowing #[pyclass(generic)] to generate __class_getitem__ for the user.

That way, we can just document that #[pyclass(generic)] is the closest we have to inheriting from typing.Generic or doing class MyExampleClass[T] on the modernized form.

@ngoldbaum
Copy link
Contributor

Fair enough about the docs! Let’s let another maintainer who knows a bit more about type hint plumbing in Python take a look at this but the FFI bits look correct to me.

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! FFI bindings look good, I have some suggestions for the safe wrapper 👍

py: Python<'py>,
origin: &Bound<'py, PyAny>,
args: &Bound<'py, PyAny>,
) -> Bound<'py, PyGenericAlias> {
Copy link
Member

Choose a reason for hiding this comment

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

The API documents that it may fail, so I think we should use PyResult return value here.

@IvanIsCoding
Copy link
Contributor Author

Thanks! FFI bindings look good, I have some suggestions for the safe wrapper 👍

I think it should handle errors now. Also, I will sitck with PyGenericAlias::new accepting PyObject instead of PyType, let's make Python handle the errors on this one.

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, just one final suggestion then.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2025
@ngoldbaum ngoldbaum added this pull request to the merge queue Feb 19, 2025
Merged via the queue into PyO3:main with commit 73ce403 Feb 19, 2025
89 checks passed
@IvanIsCoding IvanIsCoding deleted the add-generic-alias-support branch February 19, 2025 22:39
@IvanIsCoding IvanIsCoding mentioned this pull request Feb 20, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants