Skip to content

feat(types): rework of arg/return type hints to support .noconvert() #5486

New issue

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

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

Already on GitHub? # to your account

Merged
merged 28 commits into from
Jan 24, 2025

Conversation

timohl
Copy link
Contributor

@timohl timohl commented Jan 6, 2025

Description

While working on improving type hints for Eigen/numpy, I found that with the current solution from #5450 it is not possible to have the type hints be influenced by the py::arg().noconvert() option.
Certain Eigen types offer to be loaded from anything np.array() accepts which would fit to np.typing.ArrayLike.
With .noconvert() however, only instances of numpy arrays are accepted, which would fit np.typing.NDArray[<dtype>].
It would be nice if .noconvert() would force using the return type (which is the more specific type).

Meanwhile, I found that nanobind implements a similar arg/return type hint system using io_name.
This system inserts @arg-type@return-type@ using @ as a special character, which is evaluated in a later signature parsing stage.
At this stage, the .noconvert() info is available, and I was able to implement it similarly.

Additionally, I added arg_descr and return_descr to force using arg or return type hints.
This is useful for correctly type hinting typing::Callable.
The current implementation in this PR does not allow nested Callables (e.g., Callable that has a Callable as argument), but I will add support for that later this week (requires using a stack container to track nested arg_descr/return_descr).
(See 077d0b6)

Finally, I found bugs when using typing::Literal with certain special characters:
Currently, %, {, } fail with ImportError: Internal error while parsing type signature (2).
With this PR, the added special character @ fails as well when used in a Literal.
I will try to find a solution for that later this week as well.
(See 9877aca)

This also fixes #5477 by using pathlib.Path instead of Path.
(See 9d0766c)

Suggested changelog entry:

Rework of arg/return type hints to support ``.noconvert()``

Open issues:

  • Nested arg_descr/return_descr do not work (require tracking using a Stack)
  • Special characters in typing.Literal fail (%, {, }, @)
  • Fix usage of auto return type deduction (not supported in C++11)
  • Fix clang-tidy issues

Relevant nanobind code

This PR is based on the following code in nanobind:

https://github.com/wjakob/nanobind/blob/698f449a207e321b364278ff68f0b161127fd3a0/include/nanobind/nb_descr.h#L81-L86

https://github.com/wjakob/nanobind/blob/698f449a207e321b364278ff68f0b161127fd3a0/src/nb_func.cpp#L1018-L1234

@timohl
Copy link
Contributor Author

timohl commented Jan 6, 2025

Checks using C++11 fail because I used auto return type deduction which is only supported since C++14.
Will fix that soon.

@timohl
Copy link
Contributor Author

timohl commented Jan 9, 2025

This PR is now feature complete.
There is probably a lot that could be improved, but everything is working so far with added tests.

@rwgk it would be nice if you could review this, if you have time, since you helped with reviewing #5450. :)

@InvincibleRMC if this gets accepted, I would like to refresh your PR #5212.
Something similar to this would be nice to have:

io_name(
    "Annotated[np.typing.ArrayLike, \"Shape[5, 6]\"]", // type hint for args with convert=true
    "Annotated[np.typing.NDArray[np.float32], \"Shape[5, 6], flags.writeable, flags.f_contiguous\"]" // type hint for return or args with convert=false
)

The specifics could be discussed later, but having correct typing for arg/return and noconvert would be great.
What do you think about that?

@timohl
Copy link
Contributor Author

timohl commented Jan 10, 2025

I have added tests and a fix for -> in literals to prevent it from triggering the switch to return type hints while still processing arguments.
Also, I simplified some checks.

@rwgk
Copy link
Collaborator

rwgk commented Jan 14, 2025

Sorry this PR pushes the complexity of the type hint code beyond what I can meaningfully review. I don't have the free bandwidth; these changes are very far removed from my current job-role-related interests.

@wjakob could you maybe take a look? IIUC this PR more-or-less backports the nanobind io_name feature.

@timohl
Copy link
Contributor Author

timohl commented Jan 14, 2025

No problem, thanks for forwarding!

Yes, it is a backport of the io_name feature with additions to support:

  • typing.Literal - as far as I can see, nanobind does not have this typing class, so there is no problem with special characters in literals
  • typing.Callable - nanobind has type hints for std::function (see here), which might also have wrong types, but I have not tested this yet. (I can PR fixes to nanobind as well if wanted)
  • .noconvert() - nanobind also has this, so I could add io_name support for this to nanobind as well.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

It looks like it changes the way you'd set these, but I don't see usages of it in the wild, so I think it's fine and a better API.

@henryiii henryiii merged commit 1b7aa0b into pybind:master Jan 24, 2025
76 checks passed
@henryiii
Copy link
Collaborator

Thanks!

@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 24, 2025
@henryiii henryiii changed the title Rework of arg/return type hints to support .noconvert() feat(types): rework of arg/return type hints to support .noconvert() Mar 31, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Apr 1, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Wrong type annotation when using std::filesystem::path as a return type, causes stubgen to choke
3 participants