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 type hints for args and kwargs #5357

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gentlegiantJGC
Copy link
Contributor

@gentlegiantJGC gentlegiantJGC commented Sep 6, 2024

Description

The stubs generated by pybind11 do not contain any type hints for *args or **kwargs which makes static type checkers like mypy sad.

This pull requests

  1. Adds a default type hint of typing.Any to *args and **kwargs (*args: typing.Any, **kwargs: typing.Any)
  2. Adds templated classes py::Args and py::KWArgs which can be used in place of py::args and py::kwargs to add custom type hints.

Examples

#include <pybind11/pybind11.h>

void init_module(py::module m){
    m.def("test", [](py::args args, py::kwargs kwargs){});
}

Currently this will generate stub files that look like this

def test(*args, **kwargs) -> None: ...

If this is used with mypy it will generate this error.
error: Function is missing a type annotation for one or more arguments

The changes proposed will generate stub files that look like this which resolves the mypy error.

def test(*args: typing.Any, **kwargs: typing.Any) -> None: ...

Alternatively the py::Args and py::KWArgs classes can be used to add better type hints in the same vein as pybind11 does typing.

#include <pybind11/pybind11.h>

void init_module(py::module m){
    m.def("test", [](py::Args<int> args, py::KWArgs<float> kwargs){});
}

This generates the following stub.

def test(*args: int, **kwargs: float) -> None: ...

This can also be used with anything pybind11 supports including custom classes added through pybind11.

Alternatives

The only alternative solution I am currently aware of is to manually write the definition in a doc like this.

#include <pybind11/pybind11.h>

void init_module(py::module m){
    py::options options;
    options.disable_function_signatures();
    m.def("test", [](py::args args, py::kwargs kwargs){},
    py::doc("test(*args: int, **kwargs: float) -> None")
    );
    options.enable_function_signatures();
}

Suggested changelog entry:

Added default type hint for `*args` and `**kwargs`.
Added `py::Args` and `py::KWArgs` to enable custom type hinting of `*args` and `**kwargs`.

@gentlegiantJGC
Copy link
Contributor Author

The tests are failing because I haven't updated them.
If this is something you would consider merging I can update them.

@rwgk
Copy link
Collaborator

rwgk commented Sep 6, 2024

which makes static type checkers like mypy sad.

Do you have "before" and "after" this PR examples? Could you please add to the PR description?

Could you describe in more detail what's better after this PR?

@gentlegiantJGC
Copy link
Contributor Author

which makes static type checkers like mypy sad.

Do you have "before" and "after" this PR examples? Could you please add to the PR description?

Could you describe in more detail what's better after this PR?

I have updated the post.

@Skylion007
Copy link
Collaborator

typing Any isn't the right typing for this anyway, typing_extensions has specific typing args for kwargs and args params, doesn't it?

@rwgk
Copy link
Collaborator

rwgk commented Sep 15, 2024

Sorry I somehow missed your reply last week.

Quoting from the updated PR description:


Currently this will generate stub files that look like this

def test(*args, **kwargs) -> None: ...

If this is used with mypy it will generate this error.

error: Function is missing a type annotation for one or more arguments

The generated stub looks good to me, but I agree the mypy error is annoying.

What's the ideal desired behavior, assuming we can change both pybind11 and mypy?

My mind is going to: The generated stub is exactly what I'd want to see, we just need a way to express that that's intentional here, so that mypy does not complain.

@gentlegiantJGC
Copy link
Contributor Author

typing_extensions has specific typing args for kwargs and args params, doesn't it?

From what I can see it has ParamSpecArgs and ParamSpecKwargs but from the typing docs They are intended for runtime introspection and have no special meaning to static type checkers. I can't see anything else.

typing Any isn't the right typing for this anyway

What type hint would you suggest? I tried typing it as object but that lead to a compile error on your CI so I assumed that was wrong.

Here is the section in PEP 484 about typing hinting *args and **kwargs
https://peps.python.org/pep-0484/#arbitrary-argument-lists-and-default-argument-values

def test(*args, **kwargs) -> None: ...

The type hints here are implicitly Any but mypy doesn't like implicit type hints hence the error. The form I converted it to is the explicit form.

A workaround could be to add # type: ignore to the end of the function definition to suppress the mypy error but this is kind of janky and I don't know what side effects it may have if you use *args or **kwargs as part of a larger function definition.

@gentlegiantJGC
Copy link
Contributor Author

I have created #5381 which includes only the part allowing subclasses of args and kwargs.

Hopefully this will be easier to merge and allow me to implement a workaround until you can find a solution for this.

@gentlegiantJGC
Copy link
Contributor Author

A less intrusive workaround would be to leave the type hints for py::args and py::kwargs as they were and add type hints for the subclasses. That way type hints can be opt-in.
This does mean that mypy would error for the normal variants but that issue can be worked out at a later date.
Once #5396 is solved I will look into this.

@gentlegiantJGC
Copy link
Contributor Author

@rwgk this is ready for review again.
I have removed the type hint from py::args and py::kwargs so that type hints are opt-in rather than forcing it onto existing codebases.

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.

3 participants