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

Merged
merged 8 commits into from
Nov 11, 2024
Merged

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 request 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 for *args and **kwargs as described in PEP 484

Examples

#include <pybind11/pybind11.h>

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

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 py::Args and py::KWArgs classes can be used instead to add 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 `py::Args` and `py::KWArgs` to enable custom type hinting of `*args` and `**kwargs` (see PEP 484).

@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.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I finally found a few minutes to look.

Could you please help me understand?

From the PR description:

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: ...

Is this still relevant? (If not, could you please update the PR description?)

Also from the PR description:

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

What do the int and float type hints mean here? What are type checkers meant to do here? Enforce that all positional arguments have type int, and all keyword arguments have type float?

include/pybind11/pytypes.h Show resolved Hide resolved
@gentlegiantJGC
Copy link
Contributor Author

Is this still relevant?

I have removed this.

What do the int and float type hints mean here? What are type checkers meant to do here? Enforce that all positional arguments have type int, and all keyword arguments have type float?

Yes. I have edited the OP to include a link to the PEP where this is defined.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Yes. I have edited the OP to include a link to the PEP where this is defined.

I see you also added this as a comment, perfect, thanks!

@rwgk rwgk merged commit 6d98d4d into pybind:master Nov 11, 2024
81 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 11, 2024
@gentlegiantJGC gentlegiantJGC deleted the arg-type-hints branch November 12, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants